OCPBUGS-15934: use *resource.Quantity to not automatically set 0 by QiWang19 · Pull Request #3815 · openshift/machine-config-operator

QiWang19 · 2023-07-21T22:15:21Z

close: https://issues.redhat.com/browse/OCPBUGS-15934
- What I did
Change the type of OverlaySize and LogSizeMax resource.Quantity to pointer *resource.Quantity.
the struct type will be set as 0 automatically when retrieving the ctrcfg object.

I am uncertain if we can make this update to current ContainerruntimeConfig API, I propose this change because the ContainerruntimeConfig API is currently MCO internal.
- How to verify it

# apply the containerruntimeconfig CR
apiVersion: machineconfiguration.openshift.io/v1
kind: ContainerRuntimeConfig
metadata:
 name: pidlimit
spec:
 machineConfigPoolSelector:
   matchLabels:
     pools.operator.machineconfiguration.openshift.io/worker: '' 
 containerRuntimeConfig:
   pidsLimit: 4096 
   logLevel: debug

# current result:
$ oc get containerruntimeconfig  pidlimit -o json | jq '.spec.containerRuntimeConfig'
{
  "logLevel": "debug",
  "logSizeMax": "0",
  "overlaySize": "0",
  "pidsLimit": 4096
}

# after this patch, it will be 
$ oc get containerruntimeconfig  pidlimit -o json | jq '.spec.containerRuntimeConfig'
{
  "logLevel": "debug",
  "pidsLimit": 4096
}

- Description for the changelog

openshift-ci · 2023-07-21T22:15:25Z

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

QiWang19 · 2023-07-23T00:48:46Z

/test all

QiWang19 · 2023-07-23T23:47:13Z

/test e2e-aws-ovn

openshift-ci-robot · 2023-07-24T16:28:05Z

@QiWang19: This pull request references Jira Issue OCPBUGS-15934, which is invalid:

expected the bug to target the "4.14.0" version, but no target version was set

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

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

Details

In response to this:

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

close: https://issues.redhat.com/browse/OCPBUGS-15934 Signed-off-by: Qi Wang <qiwan@redhat.com>

openshift-ci-robot · 2023-07-24T17:36:34Z

@QiWang19: This pull request references Jira Issue OCPBUGS-15934, which is invalid:

expected the bug to target the "4.14.0" version, but no target version was set

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

Details

In response to this:

- What I did
Change the type of resource.Quantity to pointer *resource.Quantity so the default 0 will not set by JSON marshal when retrieving the ctrcfg objects.
We can make this change because the ContainerruntimeConfig API is currently MCO internal.
- 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.

QiWang19 · 2023-07-24T17:36:43Z

/test all

openshift-ci-robot · 2023-07-24T18:11:23Z

@QiWang19: This pull request references Jira Issue OCPBUGS-15934, which is invalid:

expected the bug to target the "4.14.0" version, but no target version was set

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

Details

In response to this:

close: https://issues.redhat.com/browse/OCPBUGS-15934
- What I did
Change the type of OverlaySize and LogSizeMax resource.Quantity to pointer *resource.Quantity.
The bug retrieving the ctrcfg object, the struct type will be set as 0 automatically

I am uncertain if we can make this update to current ContainerruntimeConfig API, I propose this change because the ContainerruntimeConfig API is currently MCO internal.
- 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-robot · 2023-08-01T16:45:22Z

@QiWang19: This pull request references Jira Issue OCPBUGS-15934, 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.14.0) matches configured target version for branch (4.14.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (schoudha@redhat.com), skipping review request.

Details

In response to this:

close: https://issues.redhat.com/browse/OCPBUGS-15934
- What I did
Change the type of OverlaySize and LogSizeMax resource.Quantity to pointer *resource.Quantity.
the struct type will be set as 0 automatically when retrieving the ctrcfg object.

I am uncertain if we can make this update to current ContainerruntimeConfig API, I propose this change because the ContainerruntimeConfig API is currently MCO internal.
- 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-robot · 2023-08-01T16:49:53Z

@QiWang19: This pull request references Jira Issue OCPBUGS-15934, which is valid.

3 validation(s) were run on this bug

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

No GitHub users were found matching the public email listed for the QA contact in Jira (schoudha@redhat.com), skipping review request.

Details

In response to this:

close: https://issues.redhat.com/browse/OCPBUGS-15934
- What I did
Change the type of OverlaySize and LogSizeMax resource.Quantity to pointer *resource.Quantity.
the struct type will be set as 0 automatically when retrieving the ctrcfg object.

I am uncertain if we can make this update to current ContainerruntimeConfig API, I propose this change because the ContainerruntimeConfig API is currently MCO internal.
- How to verify it
# apply the containerruntimeconfig CR
apiVersion: machineconfiguration.openshift.io/v1
kind: ContainerRuntimeConfig
metadata:
name: pidlimit
spec:
machineConfigPoolSelector:
  matchLabels:
    pools.operator.machineconfiguration.openshift.io/worker: '' 
containerRuntimeConfig:
  pidsLimit: 4096 
  logLevel: debug

# current result:
$ oc get containerruntimeconfig  pidlimit -o json | jq '.spec.containerRuntimeConfig'
{
 "logLevel": "debug",
 "logSizeMax": "0",
 "overlaySize": "0",
 "pidsLimit": 4096
}

# after this patch, it will be 
$ oc get containerruntimeconfig  pidlimit -o json | jq '.spec.containerRuntimeConfig'
{
 "logLevel": "debug",
 "pidsLimit": 4096
}
- 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.

QiWang19 · 2023-08-01T16:50:20Z

@mtrmac could you take a look?

mtrmac

I’m afraid I’m quite unfamiliar with this code.

Looking at the history, #2494 supposedly fixed the same bug just by adding the omitempty annotations. Is it known what has changed that this is no longer sufficient? (I can’t, from a quick check, notice anything in the history of resource.Quantity changing the semantics.)

Or did that never work?

mtrmac · 2023-08-01T18:03:02Z

 			var configFileList []generatedConfigFile
 			ctrcfg := cfg.Spec.ContainerRuntimeConfig
-			if !ctrcfg.OverlaySize.IsZero() {
+			if ctrcfg.OverlaySize != nil {


Applies to all changes:

Doesn’t this change the semantics of previously-created CRs? If I understand the situation correctly, it’s possible that users create the CRs with these fields missing, and something (patchContainerRuntimeConfigs??) updates them and adds "0" field values.

In the old version, those 0 values were treated as missing, i.e. the system did exactly what the users wanted, just in a confusing way; with this PR, wouldn’t the 0 values actually start being applied?

I suppose one way to tell would be to add (uint? e2e?) tests with CRs of that kind, and ensure that both empty values and "0" values are treated as missing. (Or maybe those tests already exist? I didn’t find them though a quick grep but I didn’t spend much time looking.)

0 values will not be applied in this PR. The drop-in files will not be created if 0 values. https://github.com/openshift/machine-config-operator/pull/3815/files#diff-b2ef9db0d76c6145ec1bb5310134037bae64572ab47464d3223745c2a410d3e3R301

Oops, my mistake, I didn’t notice that logic.

OverlaySize will, AFAICS, still cause mergeConfigChanges to be called, though with no edits. I guess that doesn’t make a difference.

Aesthetically I’d prefer for the code to consistently use a single condition, so that we just have “set to a relevant value / not set to a relevant value”, not “unset / set to zero / set to non-zero” to track.

But that’s weak a code style preference, to be decided by MCO maintainers.

mtrmac · 2023-08-01T18:03:38Z

+		if err != nil {
+			t.Errorf("%s: failed with %v. should have succeeded", test.name, err)
+		}
+		require.NotContains(t, string(data), "\"overlaySize\"", "\"overlaySize\"")


Testing LogSizeMax might be worthwhile here.

Signed-off-by: Qi Wang <qiwan@redhat.com>

QiWang19 · 2023-08-02T20:16:00Z

Updated: ignore this comment. I was testing on a build based on this PR.
~~The automatically zero values are set only on some builds. I feel it caused by different json packages somewhere.~~

4.14.0-ec.4

$ oc version
Client Version: v4.2.0-alpha.0-1974-g13225e0
Kustomize Version: v5.0.1
Server Version: 4.14.0-ec.4
Kubernetes Version: v1.27.3+4aaeaec
$ oc get containerruntimeconfig.machineconfiguration.openshift.io/pidlimit -o json | jq '.spec.containerRuntimeConfig'
{
  "logLevel": "debug",
  "logSizeMax": "0",
  "overlaySize": "0",
  "pidsLimit": 4096
}

4.14.0-0.ci.:

$ oc version
Client Version: v4.2.0-alpha.0-1974-g13225e0
Kustomize Version: v5.0.1
Server Version: 4.14.0-0.ci.test-2023-08-02-171351-ci-ln-q32sdzk-latest
Kubernetes Version: v1.27.1-3201+ba1825544533d2-dirty
$ oc get containerruntimeconfig.machineconfiguration.openshift.io/pidlimit -o json | jq '.spec.containerRuntimeConfig'
{
  "logLevel": "debug",
  "pidsLimit": 4096
}

mtrmac · 2023-08-02T20:19:11Z

That’s basically the thing that worries me — if we don’t understand how the bug arose, are we certain that we fixed it?

E.g. what if the ContainerRuntimeConfig object in question is created by something not in the MCO codebase? It’s quite possible that we need the MCO change anyway (if the old bug fix was just not working), but to actually fix the reporter’s problem we might need to find and fix that other non-MCO writer.

QiWang19 · 2023-08-02T20:32:54Z

That’s basically the thing that worries me — if we don’t understand how the bug arose, are we certain that we fixed it?

E.g. what if the ContainerRuntimeConfig object in question is created by something not in the MCO codebase? It’s quite possible that we need the MCO change anyway (if the old bug fix was just not working), but to actually fix the reporter’s problem we might need to find and fix that other non-MCO writer.

Forget about my last comment. I was testing on a cluster that build from this patch.
But you are right about the ContainerRuntimeConfig object in question is created by something not in the MCO codebase

QiWang19 · 2023-08-03T18:27:40Z

The patchContainerRuntimeConfigs can add the 0 that verified through github.com/clarketm/json.Marshal bytes
https://github.com/openshift/machine-config-operator/pull/3836/files#diff-f8f04fec2662cf966bb0a38e978bcf575bcfe7d3630caf7e2a8089d7ec7e6a70R491, what's with the fix #2494

mtrmac · 2023-08-03T18:40:26Z

OK, so #2494 was not actually sufficient. Thanks!

/lgtm

QiWang19 · 2023-08-08T12:52:36Z

/retest-required

openshift-ci · 2023-08-08T15:40:39Z

@QiWang19: 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.

QiWang19 · 2023-08-08T17:34:11Z

@yuqi-zhang could you approve?

yuqi-zhang

logically seems sound. cc @jkyros in case this affects the api move at all. Also doing a

/hold

in case we want to pre-merge verify, but feel free to unhold if we feel that is not necessary

openshift-ci · 2023-08-09T00:23:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtrmac, QiWang19, yuqi-zhang

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~~ [yuqi-zhang]

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

openshift-bot · 2023-09-08T10:05:24Z

/jira refresh

The requirements for Jira bugs have changed (Jira issues linked to PRs on main branch need to target different OCP), recalculating validity.

openshift-ci-robot · 2023-09-08T10:06:40Z

@openshift-bot: This pull request references Jira Issue OCPBUGS-15934, which is invalid:

expected the bug to target the "4.15.0" version, but it targets "4.14.0" instead

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

Details

In response to this:

/jira refresh

The requirements for Jira bugs have changed (Jira issues linked to PRs on main branch need to target different OCP), recalculating validity.

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.

yuqi-zhang · 2023-09-29T20:15:23Z

/jira refresh

openshift-ci-robot · 2023-09-29T20:15:30Z

@yuqi-zhang: This pull request references Jira Issue OCPBUGS-15934, 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)

No GitHub users were found matching the public email listed for the QA contact in Jira (schoudha@redhat.com), skipping review request.

Details

In response to this:

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

yuqi-zhang · 2023-10-11T22:01:04Z

Sorry for the delay, with openshift/api#1453 and #3747, this will first require a change to openshift/api and then re-brought in.

openshift-ci-robot · 2023-10-13T03:52:37Z

@QiWang19: This pull request references Jira Issue OCPBUGS-15934. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state.

Details

In response to this:

close: https://issues.redhat.com/browse/OCPBUGS-15934
- What I did
Change the type of OverlaySize and LogSizeMax resource.Quantity to pointer *resource.Quantity.
the struct type will be set as 0 automatically when retrieving the ctrcfg object.

I am uncertain if we can make this update to current ContainerruntimeConfig API, I propose this change because the ContainerruntimeConfig API is currently MCO internal.
- How to verify it
# apply the containerruntimeconfig CR
apiVersion: machineconfiguration.openshift.io/v1
kind: ContainerRuntimeConfig
metadata:
name: pidlimit
spec:
machineConfigPoolSelector:
  matchLabels:
    pools.operator.machineconfiguration.openshift.io/worker: '' 
containerRuntimeConfig:
  pidsLimit: 4096 
  logLevel: debug

# current result:
$ oc get containerruntimeconfig  pidlimit -o json | jq '.spec.containerRuntimeConfig'
{
 "logLevel": "debug",
 "logSizeMax": "0",
 "overlaySize": "0",
 "pidsLimit": 4096
}

# after this patch, it will be 
$ oc get containerruntimeconfig  pidlimit -o json | jq '.spec.containerRuntimeConfig'
{
 "logLevel": "debug",
 "pidsLimit": 4096
}
- 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 Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 21, 2023

QiWang19 force-pushed the zeroquantity branch 3 times, most recently from 8e0f93a to 763d4d6 Compare July 23, 2023 00:48

QiWang19 changed the title ~~[WIP] check empty resource.Quantity~~ OCPBUGS-15934: use *resource.Quantity to not automatically set 0 Jul 24, 2023

OCPBUGS-15934: use *resource.Quantity to not automatically set 0

0bbdc15

close: https://issues.redhat.com/browse/OCPBUGS-15934 Signed-off-by: Qi Wang <qiwan@redhat.com>

QiWang19 force-pushed the zeroquantity branch from 763d4d6 to 0bbdc15 Compare July 24, 2023 17:32

QiWang19 marked this pull request as ready for review August 1, 2023 16:43

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 Aug 1, 2023

openshift-ci Bot requested review from dkhater-redhat and mtrmac August 1, 2023 16:43

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Aug 1, 2023

mtrmac reviewed Aug 1, 2023

View reviewed changes

test zero value overlaysize&logsizemax

9729803

Signed-off-by: Qi Wang <qiwan@redhat.com>

QiWang19 force-pushed the zeroquantity branch from 63ac043 to 9729803 Compare August 2, 2023 05:34

openshift-ci Bot assigned mtrmac Aug 3, 2023

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Aug 3, 2023

yuqi-zhang approved these changes Aug 9, 2023

View reviewed changes

openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 9, 2023

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

openshift-ci-robot added jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. and removed jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. labels Sep 8, 2023

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Sep 29, 2023

QiWang19 closed this Oct 13, 2023

QiWang19 mentioned this pull request Nov 21, 2023

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

Merged

Conversation

QiWang19 commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci Bot commented Jul 21, 2023

Uh oh!

QiWang19 commented Jul 23, 2023

Uh oh!

QiWang19 commented Jul 23, 2023

Uh oh!

openshift-ci-robot commented Jul 24, 2023

Uh oh!

openshift-ci-robot commented Jul 24, 2023

Uh oh!

QiWang19 commented Jul 24, 2023

Uh oh!

openshift-ci-robot commented Jul 24, 2023

Uh oh!

openshift-ci-robot commented Aug 1, 2023

Uh oh!

openshift-ci-robot commented Aug 1, 2023

Uh oh!

QiWang19 commented Aug 1, 2023

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

QiWang19 Aug 2, 2023

Choose a reason for hiding this comment

Uh oh!

mtrmac Aug 2, 2023

Choose a reason for hiding this comment

Uh oh!

mtrmac Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

QiWang19 commented Aug 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac commented Aug 2, 2023

Uh oh!

QiWang19 commented Aug 2, 2023

Uh oh!

QiWang19 commented Aug 3, 2023

Uh oh!

mtrmac commented Aug 3, 2023

Uh oh!

QiWang19 commented Aug 8, 2023

Uh oh!

openshift-ci Bot commented Aug 8, 2023

Uh oh!

QiWang19 commented Aug 8, 2023

Uh oh!

yuqi-zhang left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Aug 9, 2023

Uh oh!

openshift-bot commented Sep 8, 2023

Uh oh!

openshift-ci-robot commented Sep 8, 2023

Uh oh!

yuqi-zhang commented Sep 29, 2023

Uh oh!

openshift-ci-robot commented Sep 29, 2023

Uh oh!

yuqi-zhang commented Oct 11, 2023

Uh oh!

openshift-ci-robot commented Oct 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

QiWang19 commented Jul 21, 2023 •

edited

Loading

QiWang19 commented Aug 2, 2023 •

edited

Loading