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

Peerpodconfig removal #2027

Merged

Conversation

bpradipt
Copy link
Member

@bpradipt bpradipt commented Sep 6, 2024

This PR removes peerpodconfig controller and instead moves its functionality of advertising node extended resources for peerpods to CAA daemonset itself.

@bpradipt bpradipt force-pushed the peerpodconfig-removal branch 4 times, most recently from 1133cdc to 2c3a48f Compare September 6, 2024 12:36
@bpradipt bpradipt marked this pull request as ready for review September 6, 2024 12:42
@stevenhorsman stevenhorsman added the test_e2e_libvirt Run Libvirt e2e tests label Sep 6, 2024
Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

This seems like a great change in terms of removing code and simplifying things. Do you see any negatives of have the functionality integrated?
I had question from me about the limit default value and I guess we'll need a follow-up operator PR to reflect the removal as well?
Thanks!

src/cloud-api-adaptor/cmd/cloud-api-adaptor/main.go Outdated Show resolved Hide resolved
Copy link
Contributor

@mkulke mkulke left a comment

Choose a reason for hiding this comment

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

nit about the number of peerpods per node, other lgtm

@bpradipt
Copy link
Member Author

bpradipt commented Sep 6, 2024

This seems like a great change in terms of removing code and simplifying things. Do you see any negatives of have the functionality integrated?
I had question from me about the limit default value and I guess we'll need a follow-up operator PR to reflect the removal as well?

I strongly feel our default deployments should include webhook deployment along with the limit/node. Further hardening the resource management aspects as we move towards 1.0.
Operator PR will be next to remove peerpodconfig.

@mkulke @stevenhorsman what do you prefer as a good default for the number of peerpods per node?

Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

Advertise the node extended resources for peer-pods via
the cloud-api-adaptor daemonset. This removes the dependency from
peerpodconfig-controller.
The number of peer pods per node is provided via the config param
PEERPODS_LIMIT_PER_NODE. Default is 10

Signed-off-by: Pradipta Banerjee <[email protected]>
Default is set to 10 per node

Signed-off-by: Pradipta Banerjee <[email protected]>
This is needed for CAA to advertise the per node peer-pods limit

Signed-off-by: Pradipta Banerjee <[email protected]>
Remove peerpodconfig controller as its functionality is now
included in CAA itself.

Signed-off-by: Pradipta Banerjee <[email protected]>
@bpradipt
Copy link
Member Author

bpradipt commented Sep 7, 2024

Rebased and pushed, to trigger the e2e CI again.

@bpradipt
Copy link
Member Author

bpradipt commented Sep 7, 2024

The libvirt e2e test got killed due to the time it took.
As for the govulncheck, there is a vulnerability report for gob.Decoder.Decode. Wonder if we should wait for dependabot alerts and handle it accordingly.

@stevenhorsman @mkulke any thoughts?

@stevenhorsman
Copy link
Member

The libvirt e2e test got killed due to the time it took. As for the govulncheck, there is a vulnerability report for gob.Decoder.Decode. Wonder if we should wait for dependabot alerts and handle it accordingly.

@stevenhorsman @mkulke any thoughts?

If we need to bump to 1.22.7 for this fix:

Vulnerability #1: GO-2024-3106
    Stack exhaustion in Decoder.Decode in encoding/gob
  More info: https://pkg.go.dev/vuln/GO-2024-3106
  Standard library
    Found in: encoding/[email protected]
    Fixed in: encoding/[email protected]

I'm not sure if dependabot will do this as it bumps packages rather than the go version? I'm happy for this to merge without that fix as then if dependabot hasn't created anything by Monday I can manually bump and try things out?

@bpradipt
Copy link
Member Author

bpradipt commented Sep 9, 2024

The libvirt e2e test got killed due to the time it took. As for the govulncheck, there is a vulnerability report for gob.Decoder.Decode. Wonder if we should wait for dependabot alerts and handle it accordingly.
@stevenhorsman @mkulke any thoughts?

If we need to bump to 1.22.7 for this fix:

Vulnerability #1: GO-2024-3106
    Stack exhaustion in Decoder.Decode in encoding/gob
  More info: https://pkg.go.dev/vuln/GO-2024-3106
  Standard library
    Found in: encoding/[email protected]
    Fixed in: encoding/[email protected]

I'm not sure if dependabot will do this as it bumps packages rather than the go version? I'm happy for this to merge without that fix as then if dependabot hasn't created anything by Monday I can manually bump and try things out?

I didn't read that the vulnerability is in go standard library. So we will need to bump golang.

I will merge this and open a new PR to bump golang version across the project

@bpradipt bpradipt merged commit 059f642 into confidential-containers:main Sep 9, 2024
26 of 28 checks passed
@bpradipt bpradipt deleted the peerpodconfig-removal branch September 9, 2024 03:44
@bpradipt
Copy link
Member Author

bpradipt commented Sep 9, 2024

Working on the PR to update golang - #2030

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test_e2e_libvirt Run Libvirt e2e tests
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants