-
Notifications
You must be signed in to change notification settings - Fork 366
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
Upgrade K8s libraries to v0.26.4 #4935
Upgrade K8s libraries to v0.26.4 #4935
Conversation
fa0b7c9
to
3131280
Compare
3131280
to
9cc15d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM.
I added @tnqn and @luolanzone for extra reviews.
It would have been nice to keep the sets
change in a separate commit, as it would have made it easier to focus the review process on the other changes, which require more attention.
I have pushed your new codegen image (antrea/codegen:kubernetes-1.26.4
) to the Antrea Docker registry, after building it locally.
BTW, I thought you were going to include a fix for hack/make-metric-docs.sh
?
done := make(chan interface{}) | ||
go func() { | ||
defer GinkgoRecover() | ||
cfg, err = testEnv.Start() | ||
close(done) | ||
}() | ||
Eventually(done).WithTimeout(1 * time.Minute).Should(BeClosed()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this based on some official Ginkgo examples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, kind of? I found it in one of their issue, not in official documentation. I put the link of the issue in the PR description. Here is the exact example given by Ginkgo: onsi/ginkgo#882 (comment)
@@ -1957,8 +1957,10 @@ func (data *TestData) RunCommandFromPod(podNamespace string, podName string, con | |||
if err != nil { | |||
return "", "", err | |||
} | |||
ctx, cancelFn := context.WithTimeout(context.Background(), 1*time.Minute) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does that 1 minute timeout match the previous behavior of Stream
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. The context is just for avoiding resource leak for some bad streams which will hang there forever. I thought the "longest" command we have in e2e tests is the one in flowaggregator_test, creating iperf traffic for 12s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let's go with this then. We can add a context.Context
parameter to the function later if needed
I met some issues. e.g. we use linux: https://ss64.com/bash/fmt.html I doubt whether the script was ever working on macOS. The |
012bf26
to
feaad50
Compare
build/images/codegen/Dockerfile
Outdated
# workaround for safe directory issue on github actions | ||
# ref: https://github.com/actions/runner-images/issues/6775 | ||
RUN git config --global --add safe.directory /go/src/antrea.io/antrea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with the solution overall, but I think we can make it a bit nice:
- Avoid hardcoding the
/go/src/antrea.io/antrea
in the Docker file. Instead of using aRUN
instruction in the Dockefile, let's update theupdate-codegen.sh
script so that it looks like this:
set -o errexit
set -o pipefail
ANTREA_ROOT="$( cd "$( dirname "${BASH_SOURCE[0]}" )/../" && pwd )"
IMAGE_NAME="antrea/codegen:kubernetes-1.26.4"
function docker_run() {
docker pull ${IMAGE_NAME}
set -x
ANTREA_PATH="/go/src/antrea.io/antrea"
docker run --rm \
-e GOPROXY=${GOPROXY} \
-w ${ANTREA_PATH} \
-v ${ANTREA_ROOT}:${ANTREA_PATH} \
"${IMAGE_NAME}" bash -c "git config --global --add safe.directory ${ANTREA_PATH} && $@"
}
docker_run hack/update-codegen-dockerized.sh "$@"
- Add a better comment in
update-codegen.sh
to explain why this is needed. Pointing to that issue is a bit misleading as this is orthogonal to Github actions. Maybe something like this:
Recent versions of Git will not access .git directories which are owned by another user (as a security measure), unless the directories are explicitly added to a "safe" list in the Git config. When we run the Docker container, the Antrea source directory may be owned (depends on the Docker platform) by a user which is different from the container user (as the source directory is mounted from the host). If this is the case, the Git program inside the container will refuse to run. This is why we explicitly add the Antrea source directory to the list of "safe" directories. We are still looking into the possibility of running the Docker container as the "current host user".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestions. Done.
40e7d4d
to
ec550f0
Compare
* k8s.io/apimachinery/pkg/util/sets.String was deprecated, and replaced with sets.Set[string]. sets.String.List() was replaced with sets.List(sets.Set[string]). Similar changes happened on sets.Int32, sets.Int64 * k8s.io/apiserver/pkg/registry/rest.Storage interface added requirement on implementation of Destroy() method * k8s.io/component-base/metrics/prometheus/ratelimiter was removed as it was not doing anything. The call in third_party/ipam/nodeipam was removed accordingly. Refer to PR: kubernetes/kubernetes#113054 * upgraded ginkgo to v2 as required by sigs.k8s.io/controller-runtime@v0.14.6. timeout was removed from BeforeSuite, and was refactored referencing to onsi/ginkgo#882 * k8s.io/client-go/tools/remotecommand.Executor.Stream was deprecated, and replaced with StreamWithContext Related to antrea-io#4901 Signed-off-by: heanlan <hanlan@vmware.com>
Signed-off-by: heanlan <hanlan@vmware.com>
Signed-off-by: heanlan <hanlan@vmware.com>
ec550f0
to
1ec6ac7
Compare
/test-all |
@heanlan please open a new issue for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for the review @antoninbas . Is it ok for us to merge this PR before Antrea 1.12? or we want to do it after the release |
I think we should be able to merge it, but I will let @tnqn make that call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the work.
* k8s.io/apimachinery/pkg/util/sets.String was deprecated, and replaced with sets.Set[string]. sets.String.List() was replaced with sets.List(sets.Set[string]). Similar changes happened on sets.Int32, sets.Int64 * k8s.io/apiserver/pkg/registry/rest.Storage interface added requirement on implementation of Destroy() method * k8s.io/component-base/metrics/prometheus/ratelimiter was removed as it was not doing anything. The call in third_party/ipam/nodeipam was removed accordingly. Refer to PR: kubernetes/kubernetes#113054 * upgraded ginkgo to v2 as required by sigs.k8s.io/controller-runtime@v0.14.6. timeout was removed from BeforeSuite, and was refactored referencing to onsi/ginkgo#882 * k8s.io/client-go/tools/remotecommand.Executor.Stream was deprecated, and replaced with StreamWithContext * make codegen and manifests * allow access from container users to git directories Related to antrea-io#4901 Signed-off-by: heanlan <hanlan@vmware.com>
sets.Set[string]. sets.String.List() was replaced with sets.List(sets.Set[string]).
Similar changes happened on sets.Int32, sets.Int64
implementation of Destroy() method
not doing anything. The call in third_party/ipam/nodeipam was removed
accordingly. Refer to PR: remove rate limiter metric as it is not in use kubernetes/kubernetes#113054
timeout was removed from BeforeSuite, and was refactored referencing to
Timeout on setup nodes in ginkgo v2 onsi/ginkgo#882
replaced with StreamWithContext
Related to #4901
Signed-off-by: heanlan hanlan@vmware.com