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

k8s: Bump k8s libraries to v0.30.0 #11840

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RamLavi
Copy link
Contributor

@RamLavi RamLavi commented May 2, 2024

What this PR does

This PR bumps k8s vendor to v0.30.0 on go.mod files.
It also performs needed changes in order to comply with the new vendor libraries.

Note: This PR follows the update-k8s-dependencies. manual.

Fixes #

Why we need it and why it was done in this way

The following tradeoffs were made:

The following alternatives were considered:

Links to places where the discussion took place:

Special notes for your reviewer

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Release note

Bump k8s deps to 0.30.0

@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/XXL labels May 2, 2024
@kubevirt-bot kubevirt-bot added area/virtctl sig/buildsystem Denotes an issue or PR that relates to changes in the build system. sig/observability Denotes an issue or PR that relates to observability. sig/scale sig/storage release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 2, 2024
@@ -780,7 +780,7 @@ func (app *virtAPIApp) Compose() {
}

func (app *virtAPIApp) ConfigureOpenAPIService() {
config := openapi.CreateConfig()
config := openapi.CreateV3Config()
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the time it looks like it was necessary but on second look I think it can be removed. DONE

@@ -39,7 +39,7 @@ func DefaultLeaderElectionConfiguration() Configuration {
LeaseDuration: metav1.Duration{Duration: DefaultLeaseDuration},
RenewDeadline: metav1.Duration{Duration: DefaultRenewDeadline},
RetryPeriod: metav1.Duration{Duration: DefaultRetryPeriod},
ResourceLock: resourcelock.EndpointsLeasesResourceLock,
ResourceLock: resourcelock.LeasesResourceLock,
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

@RamLavi RamLavi May 5, 2024

Choose a reason for hiding this comment

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

See here. Looks like that EndpointsLeasesResourceLock is deprecated.

Copy link
Contributor Author

@RamLavi RamLavi May 5, 2024

Choose a reason for hiding this comment

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

But for some reason the virt-controller endpoint is gone... trying to figure out why

Copy link
Contributor Author

@RamLavi RamLavi May 6, 2024

Choose a reason for hiding this comment

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

Well evidently this change made it so that the leader info is kept on the lease.
Fixed accordingly.
Added a comment on the commit desc.

@@ -1,6 +1,8 @@
module kubevirt.io/client-go

go 1.21
go 1.22.0
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I saw a runtime log saying that it needed it.. will try to remove it back and see if I get it again.

Copy link
Contributor Author

@RamLavi RamLavi May 5, 2024

Choose a reason for hiding this comment

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

go: module ./staging/src/kubevirt.io/client-go requires go >= 1.22.0; switching to go1.22.2

Added a comment on the commit desc.

"description": "A selector to restrict the list of returned objects by their fields. Defaults to everything.",
"name": "fieldSelector",
"in": "query"
"$ref": "#/parameters/fieldSelector-xIcQKXFG"
Copy link
Contributor

Choose a reason for hiding this comment

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

swagger seems wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, sure looks odd. I counted about 19 of these. make apidoc passes. still checking..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tend to think it's OK. See here .
When I look at the output with the IDE, I don't see an issue though admittedly I am no expert.
@oshoval - WDYT? Does the output look OK to you now?

Copy link
Contributor

Choose a reason for hiding this comment

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

If IDE parse it then it sounds good
I am not familiar with it, better to ask someone who does
thanks

@RamLavi RamLavi force-pushed the bump_k8s_vendor_v0.30.0 branch 3 times, most recently from 8de6611 to f97cee5 Compare May 6, 2024 17:49
@RamLavi
Copy link
Contributor Author

RamLavi commented May 7, 2024

/retest-required

Copy link
Contributor

@jean-edouard jean-edouard left a comment

Choose a reason for hiding this comment

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

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jean-edouard

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:

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

@kubevirt-bot kubevirt-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 7, 2024
@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label May 20, 2024
@kubevirt-commenter-bot
Copy link

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.30-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator
/test pull-kubevirt-e2e-k8s-1.29-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-storage
/test pull-kubevirt-e2e-k8s-1.29-sig-compute
/test pull-kubevirt-e2e-k8s-1.29-sig-operator

@RamLavi
Copy link
Contributor Author

RamLavi commented May 20, 2024

/test pull-kubevirt-unit-test
passes locally, could be flake

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@akrejcir
Copy link
Contributor

This will make it more convenient to import client-go in other projects, thanks.

A question out of curiosity: Is it ok to make kubevirt depend on k8s 1.30, when CI still runs on 1.28 and 1.29? I assumed that we support these older versions. Is the k8s.io code backward compatible?

@brianmcarey
Copy link
Member

@RamLavi - there looks to be an issue with the sig-monitoring lane on this PR. https://prow.ci.kubevirt.io/pr-history/?org=kubevirt&repo=kubevirt&pr=11840

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@RamLavi
Copy link
Contributor Author

RamLavi commented May 21, 2024

@RamLavi - there looks to be an issue with the sig-monitoring lane on this PR. https://prow.ci.kubevirt.io/pr-history/?org=kubevirt&repo=kubevirt&pr=11840

Lemme take a look
/hold

@kubevirt-bot kubevirt-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 21, 2024
@fossedihelm
Copy link
Contributor

This can fix it: #11950

@RamLavi
Copy link
Contributor Author

RamLavi commented May 22, 2024

This will make it more convenient to import client-go in other projects, thanks.

A question out of curiosity: Is it ok to make kubevirt depend on k8s 1.30, when CI still runs on 1.28 and 1.29? I assumed that we support these older versions. Is the k8s.io code backward compatible?

I don't think it's a problem, as long as we make sure to use GAed features and resources, then k8s should be backwards compatible.
Regardless, when inserting a new feature on kubevirt, one needs to make sure they take into account kubevirt's three k8s version stability constraint. This is why we have lanes to check this is true.

@oshoval
Copy link
Contributor

oshoval commented May 22, 2024

some cosmetics nits here that can be done for leader election change (your change is right, just some var name changes if you want to take a look please)
f6eaf62

Following manual [0] updating k8s vendor on all relevant go.mod files.

Additional to the normal vendor bumping, the following changes were made
to the code:
- go.mod's go version changed to go 1.22.0 (automatically by
deps-update)
- Changed PersistentVolumeClaimSpec's Resources field to be of type
VolumeResourceRequirements [1].
- DefaultLeaderElectionConfigurations ResourceLock in leader election
config was changed to LeasesResourceLock [2]. Accordingly, the
leader pod is now taken from the lease object.
- Currently builderv3 is used to create the spec for routes. Since the
spec is different now - moving to use v1.
- Move from deprecated PollImmediate.

[0]
https://github.com/kubevirt/kubevirt/blob/main/docs/update-k8s-dependencies.md
[1] https://www.pulumi.com/registry/packages/kubernetes/api-docs/core/v1/persistentvolumeclaim/#resources_nodejs
[2] https://github.com/kubevirt/kubevirt/blob/76bf9b2675954499165bc35ba12c8f9c9f45bc01/pkg/virt-controller/leaderelectionconfig/config.go#L37

Signed-off-by: Ram Lavi <ralavi@redhat.com>
@kubevirt-bot kubevirt-bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 27, 2024
@RamLavi
Copy link
Contributor Author

RamLavi commented May 27, 2024

Change: Rebase

@fossedihelm
Copy link
Contributor

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label May 28, 2024
@kubevirt-commenter-bot
Copy link

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.30-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator
/test pull-kubevirt-e2e-k8s-1.29-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-storage
/test pull-kubevirt-e2e-k8s-1.29-sig-compute
/test pull-kubevirt-e2e-k8s-1.29-sig-operator

@oshoval
Copy link
Contributor

oshoval commented May 28, 2024

if the failures are not related please consider unhold

@RamLavi
Copy link
Contributor Author

RamLavi commented May 29, 2024

if the failures are not related please consider unhold

I don't think they're related, I see very similar tests fail on other PRs (like #11516, see failing here)

/hold cancel

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 29, 2024
@brianmcarey
Copy link
Member

Looking at the PR history here - it looks like this will be very unlikely to merge until #11950 is merged.

https://prow.ci.kubevirt.io/pr-history/?org=kubevirt&repo=kubevirt&pr=11840

The hold should probably stay in place until #11950 is merged or we decide to override the monitoring lane here?

@oshoval
Copy link
Contributor

oshoval commented May 29, 2024

but if you override, wont this flake affect everything once this PR is merged?
otherwise sgtm

@RamLavi
Copy link
Contributor Author

RamLavi commented May 29, 2024

/hold
I may need to bump to v0.30.1

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 29, 2024
@kubevirt-bot
Copy link
Contributor

@RamLavi: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubevirt-e2e-kind-1.27-vgpu f87f7cc link false /test pull-kubevirt-e2e-kind-1.27-vgpu
pull-kubevirt-check-tests-for-flakes 342c201 link false /test pull-kubevirt-check-tests-for-flakes
pull-kubevirt-e2e-k8s-1.29-sig-monitoring 342c201 link true /test pull-kubevirt-e2e-k8s-1.29-sig-monitoring

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-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/virtctl dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/buildsystem Denotes an issue or PR that relates to changes in the build system. sig/network sig/observability Denotes an issue or PR that relates to observability. sig/scale sig/storage size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet