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.

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

Bugfix: MachineInfo Clone() to also clone "SwapCapacity" #3293

Merged
merged 2 commits into from
Jun 5, 2023

Conversation

iholder101
Copy link
Contributor

This PR fixes a small bug in MachineInfo's Clone() method.

Today, SwapCapacity is not being cloned, therefore after cloning it is being set to 0.

@google-cla
Copy link

google-cla bot commented Apr 16, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@k8s-ci-robot
Copy link
Collaborator

Hi @iholder101. Thanks for your PR.

I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

iholder101 added a commit to iholder101/kubernetes that referenced this pull request Apr 16, 2023
PR for the fix at upstream cAdvisor:
google/cadvisor#3293

Signed-off-by: Itamar Holder <[email protected]>
@Creatone
Copy link
Collaborator

/ok-to-test

@SergeyKanzhelev
Copy link
Collaborator

closing and reopening to recalculate CLA

@SergeyKanzhelev
Copy link
Collaborator

@iholder101 can you please sign CLA?

@SergeyKanzhelev
Copy link
Collaborator

Would you be OK writing a small unit test for the Clone method?

@iholder101
Copy link
Contributor Author

@iholder101 can you please sign CLA?

Yes, I'm currently waiting for answers regarding what's the right way to do so.

Would you be OK writing a small unit test for the Clone method?

Of course! Will do soon

@iholder101 iholder101 force-pushed the bugfix/clone-machine-info branch from cd3c6bd to 6e37e97 Compare May 16, 2023 08:27
@iholder101
Copy link
Contributor Author

Hey @SergeyKanzhelev!

I've added a unit test.
To ensure further fields won't be missed in the future, I've used reflection to fail the test if one or more fields aren't populated.

For example, if I avoid populating SwapCapacity in getFakeMachineInfo() function, the test fails with the following message:

=== RUN   TestMachineInfoClone
    machine_test.go:31: 
        	Error Trace:	cadvisor/info/v1/machine_test.go:31
        	Error:      	Should be false
        	Test:       	TestMachineInfoClone
        	Messages:   	field SwapCapacity is not populated
--- FAIL: TestMachineInfoClone (0.00s)

@iholder101 iholder101 force-pushed the bugfix/clone-machine-info branch from 6e37e97 to 183a259 Compare May 16, 2023 08:34
@SergeyKanzhelev
Copy link
Collaborator

/lgtm

Add a unit test that ensures that Clone()
method clones all fields and that the cloned
struct equals the original.

To avoid missing fields in the future, the test
fails if one of the struct's fields is populated
with a zero-value. The unit test uses reflection
to achieve that.

Signed-off-by: Itamar Holder <[email protected]>
@iholder101 iholder101 force-pushed the bugfix/clone-machine-info branch from 183a259 to 9dfa5dc Compare May 17, 2023 17:30
@iholder101
Copy link
Contributor Author

Hey @SergeyKanzhelev!

Thanks for the review and LGTM.
Would you mind pinging to an approver to have another look at this?

Copy link
Collaborator

@dims dims left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@SergeyKanzhelev SergeyKanzhelev merged commit 60785f0 into google:master Jun 5, 2023
iholder101 added a commit to iholder101/kubernetes that referenced this pull request Jun 13, 2023
PR for the fix at upstream cAdvisor:
google/cadvisor#3293

Signed-off-by: Itamar Holder <[email protected]>
iholder101 added a commit to iholder101/kubernetes that referenced this pull request Jun 28, 2023
PR for the fix at upstream cAdvisor:
google/cadvisor#3293

When a release comes out it would be possible
to bump cadvisor's version and get rid of this
commit, which is intended to be temporary.

Signed-off-by: Itamar Holder <[email protected]>
@fabiand
Copy link

fabiand commented Jun 30, 2023

@dims can we consider an out-of-cycle (or pulled up) release of cadvisor in order to increase the chances for swap to land in Kube?
IOW we request an early release with this fix in order to make sure that we can line up all the work in Kube itself to get this work merged in time. Thanks

@fabiand
Copy link

fabiand commented Jun 30, 2023

cc @SergeyKanzhelev @dchen1107

@dims
Copy link
Collaborator

dims commented Jun 30, 2023

@fabiand i am supportive, @bobbypage is the one who does releases, so please check with him.

@fabiand
Copy link

fabiand commented Jun 30, 2023

Thanks @dims
@bobbypage please share your thoughts :)

@iholder101
Copy link
Contributor Author

Thanks @dims @bobbypage please share your thoughts :)

Gentle ping. @bobbypage :)
The deadline for feature freeze (July 19th) comes fast, and this blocks my PR from getting in.

iholder101 added a commit to iholder101/kubernetes that referenced this pull request Jul 6, 2023
PR for the fix at upstream cAdvisor:
google/cadvisor#3293

When a release comes out it would be possible
to bump cadvisor's version and get rid of this
commit, which is intended to be temporary.

Signed-off-by: Itamar Holder <[email protected]>
@bobbypage
Copy link
Collaborator

@Creatone helped to cut a new v0.47.3 release with the bug fix and kubernetes/kubernetes#119225 will bump the dependency. Thanks all!

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

Successfully merging this pull request may close these issues.

7 participants