-
Notifications
You must be signed in to change notification settings - Fork 79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for KSM #1056
base: main
Are you sure you want to change the base?
Add support for KSM #1056
Conversation
This change depends on a change that failed to merge. Change openstack-k8s-operators/telemetry-operator#337 is needed. |
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
recheck |
/test openstack-operator-build-deploy-kuttl |
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/f3a8edbee473408fa4dc6a00933a3393 ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 00m 20s |
/test openstack-operator-build-deploy-kuttl |
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/a6512f9c32cc4c669e6eb80f44efa09b ✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 27m 18s |
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/ed3cbe5bb1164d639ec157cf429fd6ff ✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 27m 14s |
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
/retest |
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/af93f350b0244336bb58530b9512776c ✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 56m 37s |
6c09bc6
to
f36d7b8
Compare
looks good to me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abays, paramite The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
So we are adding a new image version to the struct. But it wouldn't exist for the old OpenStackVersion.Status Yaml I think. I would be unset there. I'm not sure I have tested this case where version is set to the old version, image is nil but the service is enabled. May have to wait till an upgrade to a specific service level exists to enable this service. |
/hold |
To clarify the concerns here: old struct has value set to nil (as it is net new it wouldn't have been initialized). Customer has the old target version set and they upgrade to the new operator codebase which upon reconciling will attempt to install this new service but will fail as there will be no container image. I think we need to either set the default (temporarily) to disabled or somehow guarantee they are at a specific version until the image is set. |
New changes are detected. LGTM label has been removed. |
@dprince Is the last commit acceptable? |
pkg/openstack/telemetry.go
Outdated
func ensureKsmImageDefault(version *corev1beta1.OpenStackVersion) *string { | ||
// NOTE(mmagr): for case upgrade case where KSM image is not part of OpenStackVersion | ||
// yet we temporarily need to ensure image default is passed | ||
var ksmImageTempDefault = "registry.k8s.io/kube-state-metrics/kube-state-metrics:v2.10.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll want to avoid using hard coded images as it would need to be set in RELATED_IMAGES as well to ensure it gets downloaded for offline/airgapped.
@paramite I pushed this: https://github.com/openstack-k8s-operators/telemetry-operator/pull/534 which will allow the OLM upgrade to proceed. The comments I made in this PR still stand and relate to the new image being available in the .Status of the OpenStackVersion resource until we have ksmImage set there (once the minor update process runs) |
@paramite: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/f12795b5bfc24b1881465addb4e92058 ✔️ openstack-k8s-operators-content-provider SUCCESS in 4h 55m 06s |
This patch add default image and TLS support for kube-state-metrics, a new component of Telemetry/Ceilometer resource.
Related: OBSDA-574
Related: OSPRH-1052
Closes: OSPRH-10462