OCPBUGS-5036: Restart pods if related configuration was changed #227

lobziik · 2023-02-15T13:14:58Z

To ensure that operands are using the most recent configurations,
such as cloud configs or credentials, this PR introduces checks for related config content.

This PR hash calculation for secrets and configmaps content if they are detected in deployment/daemonset pod template spec.

For deployments and daemonsets additional routine was added within the resourceApply procedure. For each reconciliation, it will calculate a hash for configmaps and secrets content which was referenced within PodTemplateSpec.

openshift-ci-robot · 2023-02-15T13:15:08Z

@lobziik: This pull request references Jira Issue OCPBUGS-5036, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.13.0) matches configured target version for branch (4.13.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @eurijon

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

To ensure that operands are using the most recent configurations,
such as cloud configs or credentials, this PR introduces checks for related config content.

This PR hash calculation for secrets and configmaps content if they are detected in deployment/daemonset pod template spec.

For deployments and daemonsets additional routine was added within the resourceApply procedure. For each reconciliation, it will calculate a hash for configmaps and secrets content which was referenced within PodTemplateSpec.

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/test-infra repository.

lobziik · 2023-02-15T14:28:25Z

/test unit

elmiko

code mostly makes sense to me, the hashing stuff can be brittle sometimes but the code and tests seem good.

i did have a question about log levels and a suggestion for the events.

pkg/controllers/resourceapply/config_hash.go

pkg/controllers/resourceapply/config_hash_test.go

pkg/controllers/resourceapply/resourceapply.go

elmiko

updates look nice to me, thanks!
/lgtm

lobziik · 2023-02-17T08:56:15Z

/cc @eurijon
/cc @JoelSpeed

Re-requesting review breaks review requests for some reason... :/

pierreprinetti · 2023-02-17T10:08:23Z

FYI e2e-openstack-ovn is only failing for a known unrelated reason.

lobziik · 2023-02-17T10:23:44Z

/test e2e-vsphere-ovn

JoelSpeed · 2023-02-17T10:07:35Z

pkg/cloud/vsphere/assets/cloud-controller-manager-deployment.yaml

+        - name: secret-vccm
+          secret:
+            secretName: {{ .globalCredsSecretName }}
+            optional: true


What is vccm?

vsphere cloud controller manager

What's the motivation behind adding this new secret, I don't see quite how that's related to the goal of this PR? Is it that the vSphere CCM normally looks up the creds but we need a signal to the logic that this secret is being used?

https://github.com/openshift/cluster-cloud-controller-manager-operator/blob/master/pkg/cloud/vsphere/assets/cloud-controller-manager-deployment.yaml#L63

ccm reads this secret, and need to reroll deployment if credentials changed. This is a sorta crutch to achieve this.

pkg/controllers/clusteroperator_controller.go

pkg/controllers/resourceapply/config_hash.go

pkg/controllers/resourceapply/config_hash_test.go

To ensure that operands are using most recent configurations, such as cloud configs or credentials, check for related config content was added. This patch introduces hash calculation for secrets and configmaps content if they are detected in deployment/daemonset pod template spec.

…anager` namespace

Add volume to indicate that ccm deployment is depending on the creds secret

Seems recent tooling updates made these tests a little slower, increase timeouts to 0.025s to reduce CI flakes.

Replace all event reasons with constants instead of using free-form strings to comply k8s conventions Ref: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#events

lobziik · 2023-02-20T09:19:40Z

/retest

openshift-ci · 2023-02-20T13:04:20Z

@lobziik: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-ccm-ovn-install	`bc681b8`	link	false	`/test e2e-aws-ccm-ovn-install`
ci/prow/e2e-azure-ovn-ccm-install	`bc681b8`	link	false	`/test e2e-azure-ovn-ccm-install`
ci/prow/e2e-vsphere-ovn	`bc681b8`	link	false	`/test e2e-vsphere-ovn`
ci/prow/e2e-gcp-ovn-ccm	`bc681b8`	link	false	`/test e2e-gcp-ovn-ccm`
ci/prow/e2e-gcp-ovn-ccm-install	`bc681b8`	link	false	`/test e2e-gcp-ovn-ccm-install`
ci/prow/e2e-azure-ovn-ccm	`bc681b8`	link	false	`/test e2e-azure-ovn-ccm`
ci/prow/e2e-openstack-ovn	`bc681b8`	link	false	`/test e2e-openstack-ovn`

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/test-infra repository. I understand the commands that are listed here.

elmiko

changes look good to me, i am also slightly confused by the vsphere secret update. i see that vsphere needs it, just seems a little confusing as part of this pr.

/lgtm

JoelSpeed

/approve

JoelSpeed · 2023-02-21T12:09:36Z

pkg/controllers/resourceapply/config_hash.go

+
+	hashSourceBytes, err := json.Marshal(hashSource)
+	if err != nil {
+		return "", fmt.Errorf("unable to marshal dependant config content into JSON: %v", err)


Nit, should be %w

openshift-ci · 2023-02-21T12:11:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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:

~~OWNERS~~ [JoelSpeed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2023-02-21T13:12:30Z

/retest-required

Remaining retests: 0 against base HEAD 6273311 and 2 for PR HEAD bc681b8 in total

openshift-ci-robot · 2023-02-21T15:21:41Z

@lobziik: All pull requests linked via external trackers have merged:

openshift/cluster-cloud-controller-manager-operator#227

Jira Issue OCPBUGS-5036 has been moved to the MODIFIED state.

In response to this:

To ensure that operands are using the most recent configurations,
such as cloud configs or credentials, this PR introduces checks for related config content.

This PR hash calculation for secrets and configmaps content if they are detected in deployment/daemonset pod template spec.

For deployments and daemonsets additional routine was added within the resourceApply procedure. For each reconciliation, it will calculate a hash for configmaps and secrets content which was referenced within PodTemplateSpec.

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/test-infra repository.

pierreprinetti · 2023-02-21T22:51:56Z

Is this going to be backported?

lobziik · 2023-02-22T13:54:40Z

/cherry-pick release-4.12

openshift-cherrypick-robot · 2023-02-22T13:55:20Z

@lobziik: #227 failed to apply on top of branch "release-4.12":

Applying: Fix misleading comment
Applying: Augment Deployments and Daemonsets with related configs hashes
Applying: Add watch for secrets and configmaps in `openshift-cloud-controller-manager` namespace
Applying: Add volume with the secret to the vsphere ccm
Applying: Get rid of deprecated sets, use generics instead
Applying: Increase resource rendering threshold to 25ms
Using index info to reconstruct a base tree...
M	pkg/cloud/cloud_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/cloud/cloud_test.go
CONFLICT (content): Merge conflict in pkg/cloud/cloud_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0006 Increase resource rendering threshold to 25ms
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.12

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/test-infra repository.

openshift-ci bot requested review from eurijon, elmiko and JoelSpeed February 15, 2023 13:15

Fix misleading comment

67013b7

pierreprinetti mentioned this pull request Feb 15, 2023

OSASINFRA-3058: openstack-rotate-cloud-credentials: Remove CCM workaround openshift/release#36406

Merged

elmiko reviewed Feb 15, 2023

View reviewed changes

lobziik requested review from elmiko and removed request for eurijon and JoelSpeed February 16, 2023 01:05

elmiko reviewed Feb 16, 2023

View reviewed changes

openshift-ci bot assigned elmiko Feb 16, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 16, 2023

openshift-ci bot requested review from eurijon and JoelSpeed February 17, 2023 08:56

JoelSpeed reviewed Feb 17, 2023

View reviewed changes

lobziik added 6 commits February 17, 2023 12:48

Add watch for secrets and configmaps in `openshift-cloud-controller-m…

5b275f1

…anager` namespace

Add volume with the secret to the vsphere ccm

9cedf75

Add volume to indicate that ccm deployment is depending on the creds secret

Get rid of deprecated sets, use generics instead

9825149

Increase resource rendering threshold to 25ms

a90d87c

Seems recent tooling updates made these tests a little slower, increase timeouts to 0.025s to reduce CI flakes.

Aling events reason with the conventions

bc681b8

Replace all event reasons with constants instead of using free-form strings to comply k8s conventions Ref: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#events

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 17, 2023

elmiko reviewed Feb 20, 2023

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2023

JoelSpeed reviewed Feb 21, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 21, 2023

openshift-merge-robot merged commit 96d7eab into openshift:master Feb 21, 2023

lobziik mentioned this pull request Feb 22, 2023

[release-4.12] OCPBUGS-7884: Restart pods if related configuration was changed #228

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-5036: Restart pods if related configuration was changed #227

OCPBUGS-5036: Restart pods if related configuration was changed #227

lobziik commented Feb 15, 2023

openshift-ci-robot commented Feb 15, 2023

lobziik commented Feb 15, 2023

elmiko left a comment

elmiko left a comment

lobziik commented Feb 17, 2023

pierreprinetti commented Feb 17, 2023

lobziik commented Feb 17, 2023

JoelSpeed Feb 17, 2023

lobziik Feb 17, 2023

JoelSpeed Feb 17, 2023

lobziik Feb 17, 2023

lobziik commented Feb 20, 2023

openshift-ci bot commented Feb 20, 2023 •

edited

Loading

elmiko left a comment

JoelSpeed left a comment

JoelSpeed Feb 21, 2023

openshift-ci bot commented Feb 21, 2023

openshift-ci-robot commented Feb 21, 2023

openshift-ci-robot commented Feb 21, 2023

pierreprinetti commented Feb 21, 2023

lobziik commented Feb 22, 2023

openshift-cherrypick-robot commented Feb 22, 2023

OCPBUGS-5036: Restart pods if related configuration was changed #227

OCPBUGS-5036: Restart pods if related configuration was changed #227

Conversation

lobziik commented Feb 15, 2023

openshift-ci-robot commented Feb 15, 2023

lobziik commented Feb 15, 2023

elmiko left a comment

Choose a reason for hiding this comment

elmiko left a comment

Choose a reason for hiding this comment

lobziik commented Feb 17, 2023

pierreprinetti commented Feb 17, 2023

lobziik commented Feb 17, 2023

JoelSpeed Feb 17, 2023

Choose a reason for hiding this comment

lobziik Feb 17, 2023

Choose a reason for hiding this comment

JoelSpeed Feb 17, 2023

Choose a reason for hiding this comment

lobziik Feb 17, 2023

Choose a reason for hiding this comment

lobziik commented Feb 20, 2023

openshift-ci bot commented Feb 20, 2023 • edited Loading

elmiko left a comment

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

JoelSpeed Feb 21, 2023

Choose a reason for hiding this comment

openshift-ci bot commented Feb 21, 2023

openshift-ci-robot commented Feb 21, 2023

openshift-ci-robot commented Feb 21, 2023

pierreprinetti commented Feb 21, 2023

lobziik commented Feb 22, 2023

openshift-cherrypick-robot commented Feb 22, 2023

openshift-ci bot commented Feb 20, 2023 •

edited

Loading