refactor: upgrade kube client version to v0.19.6. Fixes #4425, #4791 #4810

JPZ13 · 2020-12-29T21:30:55Z

Checklist:

My organization is added to USERS.md.

Running e2e tests here because my machine times out before completion

This PR:

Upgrades dependencies on kube (api, apimachinery, client-go, etc) to v0.20.1
Updates kube invocations with updated function signatures (lots of context and metav1 options)
Closes Broken import when using Argo go client #4791 and Update k8s.io/api, k8s.io/apimachinery and k8s.io/client-go #4425

JPZ13 · 2020-12-29T23:53:52Z

@alexec The log output on argo -v list doesn't say "Config loaded from file" anymore. Would changing the test to check for "CLI version" be acceptable? Also, running make codegen on my system isn't fixing the CI codegen. Do you have suggestions on how to fix?

cc @NikeNano

NikeNano · 2020-12-30T13:08:15Z

Also, running make codegen on my system isn't fixing the CI codegen. Do you have suggestions on how to fix?

Are you using the same version of swagger as argo currently uses @JPZ13 ? Seems like the definition don't follow version 2.

JPZ13 · 2020-12-30T15:44:13Z

Also, running make codegen on my system isn't fixing the CI codegen. Do you have suggestions on how to fix?

Are you using the same version of swagger as argo currently uses @JPZ13 ? Seems like the definition don't follow version 2.

@NikeNano I installed it using the make command in the argo root directory, but nothing changes. Here's my terminal output:

$ make $GOPATH/bin/swagger
make: `/Users/jpz13/go/bin/swagger' is up to date.

$ which swagger
/Users/jpz13/go/bin/swagger

$ swagger version
dev

$ make codegen
env ARGO_SECURE=false ARGO_INSECURE_SKIP_VERIFY=false ARGO_SERVER= ARGO_INSTANCEID= go run ./hack docgen
generating docs/fields.md
go run ./hack/cli
generating docs/cli
# `go generate ./...` takes around 10s, so we only run on specific packages.
go generate ./persist/sqldb ./pkg/apiclient/workflow ./server/auth ./server/auth/sso ./workflow/executor
Generating mock for: WorkflowArchive in file: mocks/WorkflowArchive.go
Generating mock for: WorkflowServiceClient in file: mocks/WorkflowServiceClient.go
Generating mock for: Gatekeeper in file: mocks/Gatekeeper.go
Generating mock for: Interface in file: mocks/Interface.go
Generating mock for: ContainerRuntimeExecutor in file: mocks/ContainerRuntimeExecutor.go
rm -Rf vendor

$ git status
On branch upgrade-kube
nothing to commit, working tree clean

NikeNano · 2020-12-30T20:41:52Z

hmm looks strange with:

$ swagger version
dev

maybe if you try to install version 2.X.

Since the error for CI / Codegen mentions:

The swagger spec at "api/openapi-spec/swagger.json" is invalid against swagger specification 2.0.

but I don't know these things ..... so guess we have to wait for @alexec to get feedback on this.

alexec · 2020-12-31T18:38:08Z

We will want/need to have the same Kubernetes client version as Argo Events. Can you please look at Upgrade go-client to v1.19 argo-events#999 (comment) ?
You can force recreation of files as follows rm api/openapi-spec/swagger.json && make api/openapi-spec/swagger.json. I'm not sure what the error is.
I think it would be absolutely fine to use context.TODO() to make things simpler, but I would not go back and change anything now

JPZ13 · 2020-12-31T23:11:31Z

@alexec Sounds good. I switched the dependencies to match Argo events. To fix the test-cli error, I think we have to upgrade the k8s dependencies here. That will fix klog. Should I skip those in the meantime and make the update to the argoproj/pkg repo?

I still can't get codegen to fix despite doing what you suggested. Feel free to pull this branch, make a commit running make codegen, and push that commit.

Also, happy new year. I'm game to pause on this and resume on the 4th if you're taking some time to relax.

alexec · 2021-01-02T19:13:36Z

Looks good. I'll ask Derek to review as he did the work for Argo Events. You need to make these changes:

test-e2e

TestGLogLevels is failiing.

I would say change k8s.io/klog to k8s.io/klog/v2 in cli.go, but that file is part of argoproj/pkg and cannot be changed.

Instead, change cmd/argo/commands/root.go as fal/ows:

import (
	"github.com/argoproj/pkg/cli"
	log "github.com/sirupsen/logrus"
	"github.com/spf13/cobra"
        // INSERT THIS IMPORT:
	"k8s.io/klog/v2"

...
	command.PersistentPreRun = func(cmd *cobra.Command, args []string) {
		if verbose {
			logLevel = "debug"
			glogLevel = 6
		}
		cli.SetLogLevel(logLevel)
                // INSERT THIS LINE:
                 klog.InitFlags(nil)
                // END 
		cli.SetGLogLevel(glogLevel)
		log.WithField("version", argo.GetVersion()).Debug("CLI version")
	}

codegen

This is failing Swagger validation. I don't think the swagger version being dev is the issue - this is my version locally.

My first hypothesis - the underlying problem is because controller-tools is pinned to v0.2.9 in go.mod. Delete lines 93 to 96, run go mod tidy then touch touch pkg/apis/workflow/v1alpha1/workflow_types.go (this will force many of the codegen targets to run), thenmake codegen , finally commit and push the changes.

If that does not fix this issue (or reveal the underlying cause), I'd like to know how the swagger.json has change. Insert git diff before swagger validate in the Makefile;

api/openapi-spec/swagger.json: $(GOPATH)/bin/swagger dist/kubeified.swagger.json
	swagger flatten --with-flatten minimal --with-flatten remove-unused dist/kubeified.swagger.json -o api/openapi-spec/swagger.json
        # INSERT THIS LINE:
        git diff
        # END 
	swagger validate api/openapi-spec/swagger.json
	go test ./api/openapi-spec

alexec

See comment.

JPZ13 · 2021-01-04T00:56:36Z

Thanks for the review @alexec. I deleted the lines in the go.mod file, ran go mod tidy, and did the other steps with no luck. Here's the output for git diff after the swagger flatten command:

diff --git a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json
index 99b43c0e..73cae18d 100644
--- a/api/openapi-spec/swagger.json
+++ b/api/openapi-spec/swagger.json
@@ -4154,10 +4154,6 @@
           "description": "Arguments extracted from the event and then set as arguments to the workflow created.",
           "$ref": "#/definitions/io.argoproj.workflow.v1alpha1.Arguments"
         },
-        "metadata": {
-          "description": "Metadata optional means to customize select fields of the workflow metadata",
-          "$ref": "#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta"
-        },
         "workflowTemplateRef": {
           "description": "WorkflowTemplateRef the workflow template to submit",
           "$ref": "#/definitions/io.argoproj.workflow.v1alpha1.WorkflowTemplateRef"

alexec · 2021-01-04T17:09:39Z

You have a panic in your tests:

�[36margo-server | �[m./dist/argo flag redefined: log_dir
�[36margo-server | �[mpanic: ./dist/argo flag redefined: log_dir
�[36margo-server | �[mgoroutine 1 [running]:
�[36margo-server | �[mflag.(*FlagSet).Var(0xc0000a0180, 0x278a660, 0x3982150, 0x22a6bf3, 0x7, 0x23049ec, 0x2f)
�[36margo-server | �[m	/opt/hostedtoolcache/go/1.13.12/x64/src/flag/flag.go:848 +0x4ae
�[36margo-server | �[mflag.(*FlagSet).StringVar(...)
�[36margo-server | �[m	/opt/hostedtoolcache/go/1.13.12/x64/src/flag/flag.go:751
�[36margo-server | �[mk8s.io/klog.InitFlags(0x0)
�[36margo-server | �[m	/home/runner/go/pkg/mod/k8s.io/klog@v1.0.0/klog.go:420 +0x7f
�[36margo-server | �[mgithub.com/argoproj/pkg/cli.SetGLogLevel(0x0)
�[36margo-server | �[m	/home/runner/go/pkg/mod/github.com/argoproj/pkg@v0.3.0/cli/cli.go:22 +0x2e
�[36margo-server | �[mgithub.com/argoproj/argo/cmd/argo/commands.NewCommand.func2(0xc0005badc0, 0xc0003c3180, 0x0, 0x8)
�[36margo-server | �[m	/home/runner/work/argo/argo/cmd/argo/commands/root.go:128 +0xa7
�[36margo-server | �[mgithub.com/spf13/cobra.(*Command).execute(0xc0005badc0, 0xc0003c3100, 0x8, 0x8, 0xc0005badc0, 0xc0003c3100)
�[36margo-server | �[m	/home/runner/go/pkg/mod/github.com/spf13/cobra@v1.0.0/command.go:826 +0x21d
�[36margo-server | �[mgithub.com/spf13/cobra.(*Command).ExecuteC(0xc000647080, 0xc000000180, 0xc000745f50, 0x4073cf)
�[36margo-server | �[m	/home/runner/go/pkg/mod/github.com/spf13/cobra@v1.0.0/command.go:950 +0x349
�[36margo-server | �[mgithub.com/spf13/cobra.(*Command).Execute(...)
�[36margo-server | �[m	/home/runner/go/pkg/mod/github.com/spf13/cobra@v1.0.0/command.go:887
�[36margo-server | �[mmain.main()
�[36margo-server | �[m	/home/runner/work/argo/argo/cmd/argo/main.go:13 +0x2b
�[36margo-server | �[mTerminating argo-server

alexec · 2021-01-04T17:12:04Z

can you try this?

If that does not fix this issue (or reveal the underlying cause), I'd like to know how the swagger.json has change. Insert git diff before swagger validate in the Makefile;

api/openapi-spec/swagger.json: $(GOPATH)/bin/swagger dist/kubeified.swagger.json
	swagger flatten --with-flatten minimal --with-flatten remove-unused dist/kubeified.swagger.json -o api/openapi-spec/swagger.json
        # INSERT THIS LINE:
        git diff
        # END 
	swagger validate api/openapi-spec/swagger.json
	go test ./api/openapi-spec

alexec · 2021-01-04T21:21:35Z

I look like you need to change your Makefile line 134 to:

-I ${GOPATH}/pkg/mod/github.com/grpc-ecosystem/grpc-gateway@v1.16.0/third_party/googleapis \

JPZ13 · 2021-01-04T22:17:05Z

I'm pretty stuck @alexec. Here's what I've tried so far:

removing lines 93 through 96 in the go.mod file
re-downloading all of the make codegen dependencies like protoc-gen-gogo
re-running make codegen

I've also placed a git diff where you requested. It seems all of the other tests are passing, including the previously failing cli test. Let me know if you think of anything else I should try.

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

JPZ13 · 2021-01-07T01:32:12Z

I've re-reviewed - just one final change please!

All good!

alexec · 2021-01-07T01:59:28Z

util/cmd/cmd.go

 	"strings"

 	log "github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
+	"k8s.io/klog"


this is why tests are failing again I think

Yeah my bad on that - pushing the commit now

alexec · 2021-01-07T01:59:49Z

util/cmd/cmd.go

+// SetGLogLevel set the glog level for the k8s go-client
+// this is taken from argoproj/pkg but uses v2 of klog here
+// to be compatible with k8s clients v0.19.x and above
+func SetGLogLevel(glogLevel int) {


minor - could go into own file

Sounds good - i'll scoot it into a separate file real quick

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

alexec · 2021-01-07T03:50:20Z

This is ready to merge, I think we now need to get v1.2 of Argo Events released so that we can import it on the ui-v3 branch. I'll speak to @whynowy about time-frames tomorrow.

simster7 · 2021-01-07T19:29:15Z

@sarabala1979 @simster7 could I get another pair of eyes on this PR please?

Will take a look today just for a sanity check :)

simster7

Thanks for doing all this work! My main question is whether we should consider saving the context on the structs instead of passing it down function signatures.

pkg/apis/workflow/v1alpha1/workflow_types.go

workflow/controller/controller.go

workflow/controller/dag.go

simster7 · 2021-01-07T23:34:36Z

workflow/controller/steps.go

@@ -267,7 +268,7 @@ func (woc *wfOperationCtx) executeStepGroup(stepGroup []wfv1.WorkflowStep, sgNod
 		if !childNode.Fulfilled() {
 			completed = false
 		} else if childNode.Completed() {
-			hasOnExitNode, onExitNode, err := woc.runOnExitNode(step.OnExit, step.Name, childNode.Name, stepsCtx.boundaryID, stepsCtx.tmplCtx)
+			hasOnExitNode, onExitNode, err := woc.runOnExitNode(ctx, step.OnExit, step.Name, childNode.Name, stepsCtx.boundaryID, stepsCtx.tmplCtx)


Same here, for example

simster7 · 2021-01-07T23:35:35Z

workflow/cron/controller.go

@@ -229,25 +230,26 @@ func (cc *Controller) syncAll() {
 			continue
 		}

-		err = cc.syncCronWorkflow(cronWf, groupedWorkflows[cronWf.UID])
+		ctx := context.Background()


Why are we using background here? I understand that the way syncAll gets called makes it so you can't pass parameters, but it would still be possible to pass it in using a higher-level function definition.

Maybe we should also add a cc.ctx or at least use a high-level def

This one might just be a typo where I didn't see that ctx is declared in the function signature. I'll remove line 233 here

JPZ13 · 2021-01-08T01:50:48Z

Thanks for doing all this work! My main question is whether we should consider saving the context on the structs instead of passing it down function signatures.

Thanks @simster7. I appreciate it. I'll make the changes you mentioned in the inline issues tomorrow

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

JPZ13 · 2021-01-08T20:42:45Z

The most recent commit addresses your line-item changes @simster7. Let me know if it all looks good

alexec · 2021-01-08T20:50:03Z

@simster7 do you want to review again? I'm happy to merge.

simster7 · 2021-01-08T20:52:12Z

@alexec good to merge

alexec · 2021-01-08T21:38:40Z

Next step after this is to change go.mod to contain `github.com/argoproj/argo/v3'. Let's push things forward!

JPZ13 force-pushed the upgrade-kube branch 3 times, most recently from 2958ab1 to d41ea06 Compare December 29, 2020 23:34

JPZ13 marked this pull request as draft December 29, 2020 23:43

JPZ13 changed the title ~~[WIP] refactor: upgrade kube client version~~ refactor: upgrade kube client version to v0.20.1 Dec 29, 2020

JPZ13 changed the title ~~refactor: upgrade kube client version to v0.20.1~~ refactor: upgrade kube client version to v0.20.1. Fixes #4425, #4791 Dec 29, 2020

JPZ13 marked this pull request as ready for review December 29, 2020 23:57

JPZ13 changed the title ~~refactor: upgrade kube client version to v0.20.1. Fixes #4425, #4791~~ refactor: upgrade kube client version to v0.19.6. Fixes #4425, #4791 Dec 31, 2020

JPZ13 marked this pull request as draft December 31, 2020 20:48

JPZ13 force-pushed the upgrade-kube branch from c27e62e to 80c60f9 Compare December 31, 2020 23:12

JPZ13 marked this pull request as ready for review January 1, 2021 00:03

alexec requested changes Jan 2, 2021

View reviewed changes

alexec self-assigned this Jan 2, 2021

alexec mentioned this pull request Jan 3, 2021

feat(dependencies): Update k8s dependencies to 19.3. Fixes #4425 #4426

Closed

6 tasks

JPZ13 force-pushed the upgrade-kube branch 2 times, most recently from 3e484bd to 3801ba3 Compare January 3, 2021 23:45

JPZ13 force-pushed the upgrade-kube branch 2 times, most recently from 4e85dde to df4ce8e Compare January 4, 2021 21:07

Set klog logger in other pkgs

86f4766

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

JPZ13 force-pushed the upgrade-kube branch from 92c3ab1 to 86f4766 Compare January 7, 2021 01:27

alexec reviewed Jan 7, 2021

View reviewed changes

JPZ13 added 3 commits January 6, 2021 21:05

Fix klog import

7a9d77c

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

Move setgloglevel to separate file

0900667

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

Fix spacing

47e3b7d

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

alexec approved these changes Jan 7, 2021

View reviewed changes

simster7 suggested changes Jan 7, 2021

View reviewed changes

JPZ13 added 2 commits January 8, 2021 15:13

Remove todos

82002bd

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

Refactor until to untilwithcontext

220bfed

Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>

simster7 approved these changes Jan 8, 2021

View reviewed changes

alexec merged commit a6301d7 into argoproj:master Jan 8, 2021

alexec added this to the v3.0 milestone Jan 8, 2021

JPZ13 deleted the upgrade-kube branch January 8, 2021 21:46

JPZ13 mentioned this pull request Jan 8, 2021

Upgrade CRDs to apiextensions.k8s.io/v1 #4765

Closed

NikeNano mentioned this pull request Jan 9, 2021

Upgrade Argo to v2.11+ kubeflow/pipelines#4553

Closed

simster7 mentioned this pull request Jan 12, 2021

Cherry-pick for v2.12.4 #4863

Closed

15 tasks

JPZ13 mentioned this pull request Feb 17, 2022

REQUEST: New membership for JPZ13 argoproj/argoproj#113

Closed

5 tasks

adrianord mentioned this pull request Apr 28, 2022

Full CRDs are incomplete and causes json2jsii to fail #8532

Closed

crenshaw-dev mentioned this pull request Nov 2, 2023

REQUEST: Promotion to reviewer for JPZ13 argoproj/argoproj#186

Open

5 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: upgrade kube client version to v0.19.6. Fixes #4425, #4791 #4810

refactor: upgrade kube client version to v0.19.6. Fixes #4425, #4791 #4810

JPZ13 commented Dec 29, 2020 •

edited

Loading

JPZ13 commented Dec 29, 2020 •

edited

Loading

NikeNano commented Dec 30, 2020 •

edited

Loading

JPZ13 commented Dec 30, 2020

NikeNano commented Dec 30, 2020

alexec commented Dec 31, 2020

JPZ13 commented Dec 31, 2020

alexec commented Jan 2, 2021 •

edited

Loading

alexec left a comment

JPZ13 commented Jan 4, 2021

alexec commented Jan 4, 2021

alexec commented Jan 4, 2021

alexec commented Jan 4, 2021

JPZ13 commented Jan 4, 2021

JPZ13 commented Jan 7, 2021

alexec Jan 7, 2021

alexec Jan 7, 2021

JPZ13 Jan 7, 2021

alexec Jan 7, 2021

JPZ13 Jan 7, 2021

alexec commented Jan 7, 2021

simster7 commented Jan 7, 2021

simster7 left a comment

simster7 Jan 7, 2021

simster7 Jan 7, 2021

JPZ13 Jan 8, 2021

JPZ13 commented Jan 8, 2021

JPZ13 commented Jan 8, 2021

alexec commented Jan 8, 2021

simster7 commented Jan 8, 2021

alexec commented Jan 8, 2021

refactor: upgrade kube client version to v0.19.6. Fixes #4425, #4791 #4810

refactor: upgrade kube client version to v0.19.6. Fixes #4425, #4791 #4810

Conversation

JPZ13 commented Dec 29, 2020 • edited Loading

JPZ13 commented Dec 29, 2020 • edited Loading

NikeNano commented Dec 30, 2020 • edited Loading

JPZ13 commented Dec 30, 2020

NikeNano commented Dec 30, 2020

alexec commented Dec 31, 2020

JPZ13 commented Dec 31, 2020

alexec commented Jan 2, 2021 • edited Loading

test-e2e

codegen

alexec left a comment

Choose a reason for hiding this comment

JPZ13 commented Jan 4, 2021

alexec commented Jan 4, 2021

alexec commented Jan 4, 2021

alexec commented Jan 4, 2021

JPZ13 commented Jan 4, 2021

JPZ13 commented Jan 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexec commented Jan 7, 2021

simster7 commented Jan 7, 2021

simster7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JPZ13 commented Jan 8, 2021

JPZ13 commented Jan 8, 2021

alexec commented Jan 8, 2021

simster7 commented Jan 8, 2021

alexec commented Jan 8, 2021

JPZ13 commented Dec 29, 2020 •

edited

Loading

JPZ13 commented Dec 29, 2020 •

edited

Loading

NikeNano commented Dec 30, 2020 •

edited

Loading

alexec commented Jan 2, 2021 •

edited

Loading