Skip to content

[release-4.8] Bug 2089971: lib/resourcemerge: handle container env var deletions#3161

Merged
openshift-merge-robot merged 2 commits intoopenshift:release-4.8from
openshift-cherrypick-robot:cherry-pick-3057-to-release-4.8
Oct 17, 2022
Merged

[release-4.8] Bug 2089971: lib/resourcemerge: handle container env var deletions#3161
openshift-merge-robot merged 2 commits intoopenshift:release-4.8from
openshift-cherrypick-robot:cherry-pick-3057-to-release-4.8

Conversation

@openshift-cherrypick-robot
Copy link
Copy Markdown

This is an automated cherry-pick of #3057

/assign yuqi-zhang

The logic in the resourcemerge functions only iterate through required
variables, meaning any removed variable is not handled.

As a fix to bug 1981549, this adds removal check for env vars, which
ensures that e.g. a removed proxy will correctly propagate to the
daemonset definition, which needs the proxy injected as an environment
variable to allow MCD to pull os image updates.

This of course is also a problem for everything else being synced via
these lib functions, but for now I only added the fix to EnvVar for
proxy, as it is the most likely issue for users to hit. If we want to
fix all other variables, we should probably also consider reworking
the resource* libraries in general, since they are outdated and error
prone.

To test, you can pause the pool, add a cluster proxy and then remove
it, checking the MCD daemonset after both steps to see the proxy being
added/removed. Interestingly, the adding of the proxy is almost instant,
whereas the removal can take up to 10 minutes due to the MCO seemingly
not resyncing (no action from proxy informer?). I am investigating that
separately.

Signed-off-by: Yu Qi Zhang <jerzhang@redhat.com>
This aims to add some initial tests for EnvVar handling specifically,
to make sure we don't regress proxy.

Other unit tests should be added as we clean up the code.

Signed-off-by: Yu Qi Zhang <jerzhang@redhat.com>
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 24, 2022

@openshift-cherrypick-robot: Bugzilla bug 2071689 has been cloned as Bugzilla bug 2089971. Retitling PR to link against new bug.
/retitle [release-4.8] Bug 2089971: lib/resourcemerge: handle container env var deletions

Details

In response to this:

[release-4.8] Bug 2071689: lib/resourcemerge: handle container env var deletions

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 openshift-ci Bot changed the title [release-4.8] Bug 2071689: lib/resourcemerge: handle container env var deletions [release-4.8] Bug 2089971: lib/resourcemerge: handle container env var deletions May 24, 2022
@openshift-ci openshift-ci Bot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels May 24, 2022
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 24, 2022

@openshift-cherrypick-robot: This pull request references Bugzilla bug 2089971, which is invalid:

  • expected dependent Bugzilla bug 2071689 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE), but it is MODIFIED instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

[release-4.8] Bug 2089971: lib/resourcemerge: handle container env var deletions

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.

Copy link
Copy Markdown
Contributor

@sinnykumari sinnykumari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/bugzilla refresh

@openshift-ci openshift-ci Bot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jun 2, 2022
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jun 2, 2022

@sinnykumari: This pull request references Bugzilla bug 2089971, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

6 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.8.z) matches configured target release for branch (4.8.z)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
  • dependent bug Bugzilla bug 2071689 is in the state CLOSED (ERRATA), which is one of the valid states (VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE))
  • dependent Bugzilla bug 2071689 targets the "4.9.z" release, which is one of the valid target releases: 4.9.0, 4.9.z
  • bug has dependents

Requesting review from QA contact:
/cc @sergiordlr

Details

In response to this:

/lgtm
/bugzilla refresh

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 openshift-ci Bot requested a review from sergiordlr June 2, 2022 08:57
@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jun 2, 2022
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jun 2, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: openshift-cherrypick-robot, sinnykumari

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 2, 2022
@sinnykumari
Copy link
Copy Markdown
Contributor

/label backport-risk-assessed
/assign @sergiordlr
/skip

@sinnykumari
Copy link
Copy Markdown
Contributor

/retest

@yuqi-zhang
Copy link
Copy Markdown
Contributor

/label backport-risk-assessed

Shouldn't be risky

@openshift-ci openshift-ci Bot added the backport-risk-assessed Indicates a PR to a release branch has been evaluated and considered safe to accept. label Jun 23, 2022
@openshift-bot
Copy link
Copy Markdown
Contributor

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci Bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 22, 2022
@rioliu-rh
Copy link
Copy Markdown

/label cherry-pick-approved

@openshift-ci openshift-ci Bot added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Oct 17, 2022
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

/retest-required

Remaining retests: 0 against base HEAD ec89823 and 2 for PR HEAD aa260b3 in total

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Oct 17, 2022

@openshift-cherrypick-robot: 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-metal-ipi-ovn-dualstack aa260b3 link false /test e2e-metal-ipi-ovn-dualstack
ci/prow/e2e-metal-ipi aa260b3 link false /test e2e-metal-ipi
ci/prow/e2e-metal-assisted aa260b3 link false /test e2e-metal-assisted
ci/prow/okd-e2e-aws aa260b3 link false /test okd-e2e-aws
ci/prow/okd-e2e-upgrade aa260b3 link false /test okd-e2e-upgrade

Full PR test history. Your PR dashboard.

Details

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.

@sinnykumari
Copy link
Copy Markdown
Contributor

/retest

@openshift-merge-robot openshift-merge-robot merged commit d78f3bf into openshift:release-4.8 Oct 17, 2022
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Oct 17, 2022

@openshift-cherrypick-robot: All pull requests linked via external trackers have merged:

Bugzilla bug 2089971 has been moved to the MODIFIED state.

Details

In response to this:

[release-4.8] Bug 2089971: lib/resourcemerge: handle container env var deletions

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. backport-risk-assessed Indicates a PR to a release branch has been evaluated and considered safe to accept. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm Indicates that a PR is ready to be merged. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants