[feature] adapt k8s v1.20 version #705

rambohe-ch · 2022-01-05T06:13:21Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:
/kind bug
/kind documentation
/kind enhancement
/kind good-first-issue
/kind feature
/kind question
/kind design
/sig ai
/sig iot
/sig network
/sig storage
/sig storage

/kind feature

What this PR does / why we need it:

update k8s version to v1.20.11, the main modify points as following:

update k8s.io/xxx(like client-go) dependency libraries version to v0.20.11
update yurt-controller-manager based on NodeLifeCycle controller in kube-controller-manager v1.20.11 version.
support v1.CSR api in yurtcsrapprover controller.

Which issue(s) this PR fixes:

Fixes #682

Special notes for your reviewer:

Does this PR introduce a user-facing change?

other Note

openyurt-bot · 2022-01-05T06:13:25Z

@rambohe-ch: GitHub didn't allow me to assign the following users: your_reviewer.

Note that only openyurtio members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:
/kind bug
/kind documentation
/kind enhancement
/kind good-first-issue
/kind feature
/kind question
/kind design
/sig ai
/sig iot
/sig network
/sig storage
/sig storage

/kind feature

What this PR does / why we need it:

update k8s version to v1.22.3, the main modify points as following:

update k8s.io/xxx(like client-go) dependency libraries version to v0.22.3

update yurt-controller-manager based on NodeLifeCycle controller in kube-controller-manager v1.22.3 version.

support v1.CertificateSigningRequest api

Which issue(s) this PR fixes:

Fixes #682

Special notes for your reviewer:

Does this PR introduce a user-facing change?
other Note

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.

rambohe-ch · 2022-01-05T06:13:45Z

/assign @qclc @Peeknut @Congrool

rambohe-ch · 2022-01-05T06:32:46Z

/hold

rambohe-ch · 2022-01-05T06:36:08Z

/assign @qclc @Peeknut @Congrool

Peeknut · 2022-01-05T06:56:05Z

pkg/controller/certificates/csrapprover.go

+		sharedInformers.Certificates().V1().CertificateSigningRequests().Informer().AddEventHandler(handler)
+		hasSynced = sharedInformers.Certificates().V1().CertificateSigningRequests().Informer().HasSynced
+		getCsr = sharedInformers.Certificates().V1().CertificateSigningRequests().Lister().Get
+	} else {


It seems this branch will never work?

Peeknut · 2022-01-05T07:06:24Z

pkg/controller/certificates/csrapprover.go

+	return v1beta1err
+}
+
+func v1Csr2v1beta1Csr(csr *certificatesv1.CertificateSigningRequest) *certificatesv1beta1.CertificateSigningRequest {


How about use conversion func like Convert_v1_CertificateSigningRequestList_To_certificates_CertificateSigningRequestList which provided by k8s?

but we need to import k8s.io/kubernetes, so we provide these funcs.

Peeknut · 2022-01-05T07:13:30Z

pkg/controller/nodelifecycle/node_lifecycle_controller.go

 	nc.evictorLock.Lock()
 	defer nc.evictorLock.Unlock()
+	if status == v1.ConditionFalse {


Maybe we don't need judgment of status there.
The use of markNodeForTainting will judge the status(in func processTaintBaseEviction).

This judgement is imported from upstream kubernetes. you can reference from pull request here: kubernetes/kubernetes#89059

Peeknut · 2022-01-05T07:16:45Z

pkg/util/certmanager/store/filestore_wrapper.go

 )

-// fileStoreWrapper is a wrapper for "k8s.io/client-go/util/certificate#FileStore"
+// fileStoreWrapper is a wrapper for "github.com/openyurtio/openyurt/pkg/util/certificate#FileStore"


why not use k8s.io/client-go? Did we modify something ?

qclc

The GetZoneKey(...) function in openyurt/pkg/controller/kubernetes/util/node/node.go has been replaced by k8s.io/component-helpers/node/topology/helps.go in the code, so we can delete this useless function.

qclc · 2022-01-05T08:40:37Z

pkg/yurtctl/util/kubernetes/util.go

@@ -90,7 +90,7 @@ var (
 	// CheckServantJobPeriod defines the time interval between two successive ServantJob statu's inspection
 	CheckServantJobPeriod = time.Second * 10
 	// ValidServerVersions contains all compatible server version
-	// yurtctl only support Kubernetes 1.12+ - 1.16+ for now
+	// yurtctl only support Kubernetes 1.12+ - 1.23+ for now
 	ValidServerVersions = []string{


This variable seems to have the same effect as the ValidServerVersions in openyurt/pkg/yurtctl/cmd/convert/options.go. How about merging the two variables together？

rambohe-ch · 2022-01-05T12:20:50Z

The GetZoneKey(...) function in openyurt/pkg/controller/kubernetes/util/node/node.go has been replaced by k8s.io/component-helpers/node/topology/helps.go in the code, so we can delete this useless function.

ok

Congrool · 2022-01-05T10:12:24Z

cmd/yurt-controller-manager/app/controllermanager.go

+				}
+			}
+			return nil
+		},


How about just testing if len(args) > 0, instead of a range loop

in order to avoid influence from empty args, so a range loop used. and these code snippet is imported from upstream kubernetes.

Congrool · 2022-01-05T16:45:28Z

go.mod

+	k8s.io/kube-controller-manager v0.22.3
+	k8s.io/kubelet v0.22.3
+	k8s.io/system-validators v1.6.0
+	k8s.io/utils v0.0.0-20210930125809-cb0fa318a74b


In require block, how about directly using k8s.io/api v0.22.3, k8s.io/klog/v2 v2.9.0 ?

@rambohe-ch BTW, cloud I know the reason why we need to require and replace the package with the same version, such as :

require ( ... k8s.io/apimachinery v0.22.3 ... ) replace ( ... k8s.io/apimachinery => k8s.io/apimachinery v0.22.3 ... )

In my mind, we just need to do so for sigs.k8s.io/apiserver-network-proxy v0.0.15, because we forked it and modified for openyurt.
Thanks.

Congrool · 2022-01-06T04:19:29Z

After this pr, how about updating the k8s version of e2e test to 1.22?

rambohe-ch · 2022-01-06T05:32:44Z

After this pr, how about updating the k8s version of e2e test to 1.22?

@Congrool Would you be able to make a pull request to update e2e test?

Congrool · 2022-01-06T05:49:44Z

After this pr, how about updating the k8s version of e2e test to 1.22?

@Congrool Would you be able to make a pull request to update e2e test?

Sure, but things seem to be more complicate than just updating kind node image.
We use both csr v1 and v1beta1 to support 1.22 and the previous version. Do we need to retain tests for 1.21 and previous version, or just only test 1.22?

qclc · 2022-01-06T10:06:29Z

/hold cancel

qclc · 2022-01-06T10:06:42Z

/lgtm

kadisi · 2022-01-06T11:43:51Z

/lgtm

kadisi · 2022-01-06T11:44:25Z

/approve

qclc · 2022-01-06T11:52:19Z

/hold

rambohe-ch · 2022-01-10T04:14:51Z

/hold cancel

qclc · 2022-01-10T10:38:04Z

/lgtm

rambohe-ch · 2022-01-10T11:16:04Z

/hold cancel

openyurt-bot · 2022-01-10T11:19:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: huangyuqi, kadisi, rambohe-ch

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:

~~OWNERS~~ [huangyuqi,kadisi,rambohe-ch]

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

huangyuqi · 2022-01-10T11:30:55Z

/lgtm

[feature] adapt k8s v1.20 version

openyurt-bot added the kind/feature kind/feature label Jan 5, 2022

openyurt-bot added the size/XXL label Jan 5, 2022

openyurt-bot requested review from adamzhoul and Peeknut January 5, 2022 06:16

openyurt-bot added the approved approved label Jan 5, 2022

rambohe-ch force-pushed the update-k8s-v1.22 branch 2 times, most recently from 7a57436 to 64e3eb9 Compare January 5, 2022 06:29

openyurt-bot added the do-not-merge/hold do-not-merge/hold label Jan 5, 2022

openyurt-bot assigned Congrool, Peeknut and qclc Jan 5, 2022

Peeknut reviewed Jan 5, 2022

View reviewed changes

qclc reviewed Jan 5, 2022

View reviewed changes

rambohe-ch force-pushed the update-k8s-v1.22 branch from 64e3eb9 to e640cfc Compare January 5, 2022 12:31

Congrool reviewed Jan 5, 2022

View reviewed changes

rambohe-ch force-pushed the update-k8s-v1.22 branch from e640cfc to 0e7d9ca Compare January 6, 2022 03:45

openyurt-bot removed the do-not-merge/hold do-not-merge/hold label Jan 6, 2022

openyurt-bot added the lgtm lgtm label Jan 6, 2022

openyurt-bot assigned kadisi Jan 6, 2022

openyurt-bot added the do-not-merge/hold do-not-merge/hold label Jan 6, 2022

rambohe-ch force-pushed the update-k8s-v1.22 branch from 0e7d9ca to 1b51ec2 Compare January 6, 2022 12:46

openyurt-bot removed the lgtm lgtm label Jan 6, 2022

rambohe-ch force-pushed the update-k8s-v1.22 branch 2 times, most recently from 22bd284 to fb99b04 Compare January 10, 2022 04:04

rambohe-ch changed the title ~~[feature] adapt k8s v1.22 version~~ [feature] adapt k8s v1.20 version Jan 10, 2022

[feature] adapt k8s v1.20.11 version

4215dc1

rambohe-ch force-pushed the update-k8s-v1.22 branch from fb99b04 to 4215dc1 Compare January 10, 2022 04:13

openyurt-bot added the lgtm lgtm label Jan 10, 2022

openyurt-bot removed the do-not-merge/hold do-not-merge/hold label Jan 10, 2022

huangyuqi approved these changes Jan 10, 2022

View reviewed changes

openyurt-bot assigned huangyuqi Jan 10, 2022

huangyuqi merged commit 0cd9d71 into openyurtio:master Jan 10, 2022

MrGirl pushed a commit to MrGirl/openyurt that referenced this pull request Mar 29, 2022

Merge pull request openyurtio#705 from rambohe-ch/update-k8s-v1.22

aab828f

[feature] adapt k8s v1.20 version

[feature] adapt k8s v1.20 version #705

[feature] adapt k8s v1.20 version #705

Conversation

rambohe-ch commented Jan 5, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

other Note

openyurt-bot commented Jan 5, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

other Note

rambohe-ch commented Jan 5, 2022 • edited

rambohe-ch commented Jan 5, 2022

rambohe-ch commented Jan 5, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qclc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rambohe-ch commented Jan 5, 2022

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

Congrool commented Jan 6, 2022

rambohe-ch commented Jan 6, 2022

Congrool commented Jan 6, 2022 • edited

qclc commented Jan 6, 2022

qclc commented Jan 6, 2022

kadisi commented Jan 6, 2022

kadisi commented Jan 6, 2022 • edited

qclc commented Jan 6, 2022

rambohe-ch commented Jan 10, 2022

qclc commented Jan 10, 2022

rambohe-ch commented Jan 10, 2022

openyurt-bot commented Jan 10, 2022

huangyuqi commented Jan 10, 2022

rambohe-ch commented Jan 5, 2022 •

edited

rambohe-ch commented Jan 5, 2022 •

edited

Congrool commented Jan 6, 2022 •

edited

kadisi commented Jan 6, 2022 •

edited