Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Bug 1791863: lib/resourcemerge/core: Clear livenessProbe and readinessProbe if nil in required #298

Merged
merged 1 commit into from Jan 17, 2020
Merged

Bug 1791863: lib/resourcemerge/core: Clear livenessProbe and readinessProbe if nil in required #298

merged 1 commit into from Jan 17, 2020

Conversation

wking
Copy link
Member

@wking wking commented Jan 16, 2020

We've claimed:

if we have no required, then we don't care what someone else has set

since this code landed in d9f6718 (#7). But we do care in situations like rhbz#1791863, where a 4.3 -> 4.2 downgrade leaves 4.3 readiness probes in place for a 4.2 operator which has no /readyz.

With this pull-request we still have a few other types where we claim to not care:

$ git grep -hB1 'someone else has set' | grep func
func ensureSecurityContextPtr(modified *bool, existing **corev1.SecurityContext, required *corev1.SecurityContext) {
func ensureCapabilitiesPtr(modified *bool, existing **corev1.Capabilities, required *corev1.Capabilities) {
func ensureAffinityPtr(modified *bool, existing **corev1.Affinity, required *corev1.Affinity) {
func ensurePodSecurityContextPtr(modified *bool, existing **corev1.PodSecurityContext, required *corev1.PodSecurityContext) {
func ensureSELinuxOptionsPtr(modified *bool, existing **corev1.SELinuxOptions, required *corev1.SELinuxOptions) {
func setBoolPtr(modified *bool, existing **bool, required *bool) {
func setInt64Ptr(modified *bool, existing **int64, required *int64) {

but I'm leaving them alone until we have a clearer picture about whether we care about those specific properties or not.

@wking
lib/resourcemerge/core: Clear livenessProbe and readinessProbe if nil…
29b92d2 
… in required

We claimed:

  if we have no required, then we don't care what someone else has set

since this code landed in d9f6718 (lib: add lib for applying
objects, 2018-08-14, openshift#7).  But we do care in situations like [1],
where a 4.3 -> 4.2 downgrade leaves 4.3 readiness probes in place for
a 4.2 operator which has no /readyz.

With this commit we still have a few other types where we claim to not
care:

  $ git grep -hB1 'someone else has set' | grep func
  func ensureSecurityContextPtr(modified *bool, existing **corev1.SecurityContext, required *corev1.SecurityContext) {
  func ensureCapabilitiesPtr(modified *bool, existing **corev1.Capabilities, required *corev1.Capabilities) {
  func ensureAffinityPtr(modified *bool, existing **corev1.Affinity, required *corev1.Affinity) {
  func ensurePodSecurityContextPtr(modified *bool, existing **corev1.PodSecurityContext, required *corev1.PodSecurityContext) {
  func ensureSELinuxOptionsPtr(modified *bool, existing **corev1.SELinuxOptions, required *corev1.SELinuxOptions) {
  func setBoolPtr(modified *bool, existing **bool, required *bool) {
  func setInt64Ptr(modified *bool, existing **int64, required *int64) {

but I'm leaving them alone until we have a clearer picture about
whether we care about those specific properties or not.

[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1791863#c1
@openshift-ci-robot
Copy link
Contributor

@wking: This pull request references Bugzilla bug 1791863, which is invalid:

  • expected the bug to target the "4.4.0" release, but it targets "---" 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.

In response to this:

Bug 1791863: lib/resourcemerge/core: Clear livenessProbe and readinessProbe if nil in required

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-robot openshift-ci-robot added bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 16, 2020
@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 16, 2020
@wking
Copy link
Member Author

wking commented Jan 16, 2020

/bugilla refresh

@wking
Copy link
Member Author

wking commented Jan 16, 2020

@crawford, @smarterclayton: can one of you take a look at this for the 4.3->4.2 rollback bug?

@wking
Copy link
Member Author

wking commented Jan 16, 2020

With this pull-request we still have a few other types where we claim to not care...

Of these types, I'd expect us to care (and clear the property) for all of those except maybe setBoolPtr and setInt64Ptr. Those are so generic it's hard to have an opinion of them on their own, but I'll go through our callers and try to get some context...

@wking
Copy link
Member Author

wking commented Jan 16, 2020

/bugilla refresh

@wking
Copy link
Member Author

wking commented Jan 16, 2020

e2e-aws-upgrade:

level=fatal msg="failed to fetch Terraform Variables: failed to fetch dependency of \"Terraform Variables\": failed to fetch dependency of \"Bootstrap Ignition Config\": failed to fetch dependency of \"Master Machines\": failed to generate asset \"Platform Credentials Check\": validate AWS credentials: checking install permissions: error simulating policy: Throttling: Rate exceeded\n\tstatus code: 400, request id: 4fd72c56-b482-4c0a-896d-54b2ddeecb73"

Which is a high-CI-load flake that we'll get past if/when this gets approved and retested.

@wking
Copy link
Member Author

wking commented Jan 16, 2020

Set up the cherry-pick chain for this:

/cherrypick release-4.3

@openshift-cherrypick-robot
Copy link

@wking: once the present PR merges, I will cherry-pick it on top of release-4.3 in a new PR and assign it to you.

In response to this:

Set up the cherry-pick chain for this:

/cherrypick release-4.3

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.

@wking
Copy link
Member Author

wking commented Jan 16, 2020

/cherrypick release-4.2

@openshift-cherrypick-robot
Copy link

@wking: once the present PR merges, I will cherry-pick it on top of release-4.2 in a new PR and assign it to you.

In response to this:

/cherrypick release-4.2

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.

@wking
Copy link
Member Author

wking commented Jan 16, 2020

/cherrypick release-4.1

@openshift-cherrypick-robot
Copy link

@wking: once the present PR merges, I will cherry-pick it on top of release-4.1 in a new PR and assign it to you.

In response to this:

/cherrypick release-4.1

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.

@wking
Copy link
Member Author

wking commented Jan 16, 2020

/bugzilla refresh

I can't spell :(

@openshift-ci-robot
Copy link
Contributor

@wking: An error was encountered searching for bug 1791863 on the Bugzilla server at https://bugzilla.redhat.com:

response code 502 not 200
Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

In response to this:

/bugzilla refresh

I can't spell :(

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.

@wking
Copy link
Member Author

wking commented Jan 16, 2020

/bugzilla refresh

@openshift-ci-robot openshift-ci-robot 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 Jan 16, 2020
@openshift-ci-robot
Copy link
Contributor

@wking: This pull request references Bugzilla bug 1791863, which is valid.

In response to this:

/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.

@jottofar
Copy link
Contributor

LGTM

@wking
Copy link
Member Author

wking commented Jan 16, 2020

e2e-aws:

level=error msg="Error: Error waiting for AMI (ami-015791a7fa2aa419e) to be ready: timeout while waiting for state to become 'available' (last state: 'pending', timeout: 40m0s)"

AWS has been really slow with AMI copies today, but we'll eventually squeak through on retest if/when this gets approved.

@smarterclayton
Copy link
Contributor

It is appropriate to keep this selective.

@smarterclayton
Copy link
Contributor

/lgtm
/retest

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 16, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton, wking

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 [smarterclayton,wking]

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

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

16 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit b89aa75 into openshift:master Jan 17, 2020
@openshift-ci-robot
Copy link
Contributor

@wking: All pull requests linked via external trackers have merged. Bugzilla bug 1791863 has been moved to the MODIFIED state.

In response to this:

Bug 1791863: lib/resourcemerge/core: Clear livenessProbe and readinessProbe if nil in required

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-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Jan 17, 2020
Merged
@openshift-cherrypick-robot
Copy link

@wking: new pull request created: #299

In response to this:

Set up the cherry-pick chain for this:

/cherrypick release-4.3

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-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Jan 17, 2020
Merged
@openshift-cherrypick-robot
Copy link

@wking: new pull request created: #300

In response to this:

/cherrypick release-4.2

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-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Jan 17, 2020
Merged
@openshift-cherrypick-robot
Copy link

@wking: new pull request created: #301

In response to this:

/cherrypick release-4.1

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.

@wking wking deleted the remove-probes-when-manifest-has-none branch April 15, 2020 04:38
wking added a commit to wking/cluster-version-operator that referenced this pull request Apr 20, 2021
@wking
lib/resourcemerge/core: Clear env and envFrom if unset in manifest
02bb9ba 
Even if the manifest authors state no opinions, these are not
properties that we want to allow cluster admins to manipulate.  For
example, a customer cluster recently stuck a Deployment by inserting a
reference to a non-existent secret [1]:

  $ yaml2json <namespaces/openshift-marketplace/apps/deployments.yaml | jq -r '.items[].spec.template.spec.containers[].envFrom[]'
  {
    "secretRef": {
      "name": "openshift-reg"
    }
  }
  $ yaml2json <namespaces/openshift-marketplace/pods/marketplace-operator-f7cc88d59-hhh75/marketplace-operator-f7cc88d59-hhh75.yaml | jq -r '.status.containerStatuses[].state'
  {
    "waiting": {
      "message": "secret \"openshift-reg\" not found",
      "reason": "CreateContainerConfigError"
    }
  }

The outgoing logic dates back to the beginning of reconciling these
properties in 14fab0b (add generic 2-way merge handler for random
types, 2018-09-27, openshift#26), and this commit's tightening follows on a
number of reconciliation tightenings like 29b92d2
(lib/resourcemerge/core: Clear livenessProbe and readinessProbe if nil
in required, 2020-01-16, openshift#298).

[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1951339#c0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhinavdahiya abhinavdahiya Awaiting requested review from abhinavdahiya

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@smarterclayton smarterclayton

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@wking @openshift-ci-robot @openshift-cherrypick-robot @jottofar @smarterclayton @openshift-bot @openshift-merge-robot