OCPBUGS-22364: Bump openshift/API to mark MCO cert observability fields optional in CRD by jkyros · Pull Request #4003 · openshift/machine-config-operator

jkyros · 2023-10-26T14:17:50Z

This bumps the MCO API to bring in the cert obvservability field changes from openshift/api#1637

( It looks like controllerConfig picked up some network-related CRD changes in the platform spec also)

All I did:

$ go get -u github.com/openshift/api
$ make go-deps
$ hack/crds-sync.sh

openshift-ci-robot · 2023-10-26T14:17:56Z

@jkyros: This pull request references Jira Issue OCPBUGS-22364, which is valid.

3 validation(s) were run on this bug

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

Requesting review from QA contact:
/cc @sergiordlr

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

Details

In response to this:

Ignore this for now, just want QE to be able to test.

- What I did

- How to verify it

- Description for the changelog

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 · 2023-10-26T14:18:29Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

jkyros · 2023-10-26T14:22:02Z

/test all

openshift-ci · 2023-10-26T14:24:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jkyros

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:

~~OWNERS~~ [jkyros]

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-11-21T21:31:01Z

@jkyros: This pull request references Jira Issue OCPBUGS-22364, which is valid.

3 validation(s) were run on this bug

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

Requesting review from QA contact:
/cc @rioliu-rh

Details

In response to this:

This bumps the MCO API to bring in the cert obvservability field changes from openshift/api#1637

( It looks like controllerConfig picked up some network-related CRD changes in the platform spec also)

All I did:

$ go get -u github.com/openshift/api

$ make go-deps

$ hack/crds-sync.sh

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.

We need to bump the API to bring in the changes that make the cert observability fields optional for now (so we can avoid the race condition where the existing MCO controllers that think the fields are optional can come into contact with a new CRD that thinks they are not). This also drags in the following MCO API changes: - some baremetal platform networking fields - the ContainerRuntimeConfig change of LogLevel and OverlaySize to pointers

verify has apparently started to care about %w being used in klog statements that don't support it, so I changed the two offending statements to use %v instead.

jkyros · 2023-11-21T22:13:26Z

Had to account for verify being pickier about %w usage and the ContainerRuntimeConfig API changes from openshift/api#1656 which this also drags in.

The LogSizeMax and OverlaySize fields in ContainerRuntimeConfig were changed to pointers in the API, and that change came with our API bump, so we need to accommodate that here in the controller test. Everything else is fine because everything else is using the .Value() and .String() methods when consuming those fields.

jkyros · 2023-11-21T23:02:15Z

yuck, unit passed locally, is that a flake?
/test unit

QiWang19 · 2023-11-21T23:17:45Z

The ci/prow/unit failure pkg/controller/container-runtime-config resource.Quantity can be fixed by the commit a8cd848
right now I am not sure about the failures of the pkg/controller/kubelet-config

openshift-ci · 2023-11-22T02:25:13Z

@jkyros: all tests passed!

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.

jkyros · 2023-11-22T03:29:01Z

Oh, heyyyy @QiWang19 it looks like you are doing the same thing I am doing....and you have more tests. Excellent timing 😄

Yeah, I had hacked in some quick nil checks + fixes to accommodate the ControllerRuntime changes, but your commit is definitely more detailed and probably better.

The flake I was referencing was weirdly seemingly not related to any of this. I got a weird unit run that failed with :

1121 22:45:59.020479   22201 render_controller.go:564] No BaseOSContainerImage set
    render_controller_test.go:151: Action update machineconfigpools has wrong object
        Diff:
           &v1.MachineConfigPool{
          	TypeMeta:   {APIVersion: "machineconfiguration.openshift.io/v1"},
          	ObjectMeta: {Name: "test-cluster-master", UID: "wc9zt", Labels: {"machineconfiguration.openshift.io/mco-built-in": "", "pools.operator.machineconfiguration.openshift.io/test-cluster-master": ""}},
          	Spec: v1.MachineConfigPoolSpec{
          		... // 2 identical fields
          		Paused:         false,
          		MaxUnavailable: nil,
          		Configuration: v1.MachineConfigPoolStatusConfiguration{
          			ObjectReference: {Name: "rendered-test-cluster-master-a39f193e46d2048532b91e0ddc0faceb"},
          			Source: []v1.ObjectReference{
          				{
          					Kind:       "MachineConfig",
          					Namespace:  "",
        - 					Name:       "00-test-cluster-master",
        + 					Name:       "05-extra-master",
          					UID:        "",
          					APIVersion: "machineconfiguration.openshift.io/v1",
          					... // 2 identical fields
          				},
          				{
          					Kind:       "MachineConfig",
          					Namespace:  "",
        - 					Name:       "05-extra-master",
        + 					Name:       "00-test-cluster-master",
          					UID:        "",
          					APIVersion: "machineconfiguration.openshift.io/v1",
          					... // 2 identical fields
          				},
          			},
          		},
          	},
          	Status: {Configuration: {ObjectReference: {Name: "rendered-test-cluster-master-a39f193e46d2048532b91e0ddc0faceb"}}, Conditions: {{Type: "RenderDegraded", Status: "False", LastTransitionTime: {Time: s"1970-01-01 00:00:00 +0000 UTC"}}}},
          }

Which felt very non-deterministic. And it never happened again, and seemed unrelated to everything.

QiWang19 · 2023-11-27T21:25:52Z

@jkyros Do you plan to merge this as part of 4.15? if so #4044 can wait after this one

jkyros · 2023-11-28T06:50:53Z

@QiWang19 Yes, I do intend to merge this as part of 4.15, I'll try to get this finished up tomorrow or Wed.

jkyros · 2023-11-28T17:12:57Z

Actually...it looks like Charlie is going to bump API as part of #4012 so that might obviate this. I'll close this if #4012 merges

jkyros · 2023-11-29T17:15:38Z

#4012 merged which bumps the API and contains these changes, closing this.

openshift-ci-robot · 2023-11-29T17:15:41Z

@jkyros: This pull request references Jira Issue OCPBUGS-22364. The bug has been updated to no longer refer to the pull request using the external bug tracker.

Details

In response to this:

This bumps the MCO API to bring in the cert obvservability field changes from openshift/api#1637

( It looks like controllerConfig picked up some network-related CRD changes in the platform spec also)

All I did:

$ go get -u github.com/openshift/api

$ make go-deps

$ hack/crds-sync.sh

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 added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Oct 26, 2023

openshift-ci-robot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Oct 26, 2023

openshift-ci Bot requested a review from sergiordlr October 26, 2023 14:18

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2023

jkyros mentioned this pull request Oct 26, 2023

OCPBUGS-22364: Make MCO certificate observability fields optional for 4.15 openshift/api#1637

Merged

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2023

rioliu-rh mentioned this pull request Oct 30, 2023

OCPBUGS-20152,OCPBUGS-22364: fix timing issues upon cluster installation or upgrade #3987

Closed

jkyros force-pushed the make-cert-fields-optional-for-415 branch from 304df47 to c6f9a43 Compare November 21, 2023 21:26

openshift-ci Bot requested a review from rioliu-rh November 21, 2023 21:31

jkyros changed the title ~~[WIP] OCPBUGS-22364: Bump openshift/API to mark MCO cert observability fields optional in CRD~~ OCPBUGS-22364: Bump openshift/API to mark MCO cert observability fields optional in CRD Nov 21, 2023

jkyros marked this pull request as ready for review November 21, 2023 21:31

openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 21, 2023

openshift-ci Bot requested review from cheesesashimi and djoshy November 21, 2023 21:31

jkyros added 3 commits November 21, 2023 16:05

Run hack/crds-sync.sh

78c607b

Remove unsupported %w in klog statements

8b3b698

verify has apparently started to care about %w being used in klog statements that don't support it, so I changed the two offending statements to use %v instead.

jkyros force-pushed the make-cert-fields-optional-for-415 branch from c6f9a43 to ee54e6c Compare November 21, 2023 22:12

jkyros added 2 commits November 21, 2023 16:40

update hard coded feature gates to match api

9c33da2

jkyros force-pushed the make-cert-fields-optional-for-415 branch from ee54e6c to 9c33da2 Compare November 21, 2023 22:40

QiWang19 mentioned this pull request Nov 22, 2023

OCPBUGS-15934: unit test ctrrcfg uses *resource.Quantity to use *resource.Quantity #4044

Merged

jkyros closed this Nov 29, 2023

jkyros mentioned this pull request Dec 14, 2023

MCO-831: added feature gate to mco for on cluster builds #4060

Merged

Conversation

jkyros commented Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Oct 26, 2023

Uh oh!

openshift-ci Bot commented Oct 26, 2023

Uh oh!

jkyros commented Oct 26, 2023

Uh oh!

openshift-ci Bot commented Oct 26, 2023

Uh oh!

openshift-ci-robot commented Nov 21, 2023

Uh oh!

jkyros commented Nov 21, 2023

Uh oh!

jkyros commented Nov 21, 2023

Uh oh!

QiWang19 commented Nov 21, 2023

Uh oh!

openshift-ci Bot commented Nov 22, 2023

Uh oh!

jkyros commented Nov 22, 2023

Uh oh!

QiWang19 commented Nov 27, 2023

Uh oh!

jkyros commented Nov 28, 2023

Uh oh!

jkyros commented Nov 28, 2023

Uh oh!

jkyros commented Nov 29, 2023

Uh oh!

openshift-ci-robot commented Nov 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jkyros commented Oct 26, 2023 •

edited

Loading