Boskos: Remove downstream client by alvaroaleman · Pull Request #16206 · kubernetes/test-infra

alvaroaleman · 2020-02-10T12:42:58Z

This PR is a first step towards making Boskos use a cache underneath, which we need to speed the whole thing up, as it lists all Resources for each Acquire/AcquireByState call, which is pretty slow because we have about 4k objects.

This PR removes the downstream kube client we have in boskos and replaces it with the one from controller-runtime, of which an api-compatible cache-backed version exists.

Note that the PR does not change the client to be cache-backed yet, I will do that in a followup as this PR is big enough already.

/assign @ixdy @sebastienvas @stevekuznetsov

Currently, controller-runtime requires list types to: * End in `List`: https://github.com/kubernetes-sigs/controller-runtime/blob/82a78f979a567e9ff215905e57082a163067d8da/pkg/client/client_cache.go#L60 * Use non-pointer types as Items: kubernetes-sigs/controller-runtime#656

ixdy

Thanks for taking this on! This change generally LG to me - just a few questions.

boskos/ranch/ranch.go

ixdy · 2020-02-10T23:36:35Z

boskos/ranch/ranch_test.go

-	"k8s.io/test-infra/boskos/crds"
 )

+// Make debugging a bit easier


how spammy is this?

The go tests suppress all logrus logs unless the test fails, in which case the person debugging them most likely wants debug-level logs.

Not a strong opinion here though, I can also remove it again.

ixdy · 2020-02-10T23:41:45Z

boskos/ranch/storage.go

 type Storage struct {
-	resources, dynamicResourceLifeCycles storage.PersistenceLayer
-	resourcesLock                        sync.RWMutex
+	ctx           context.Context


when is this set?

Never, added a commit to properly plug it through

ixdy · 2020-02-10T23:52:11Z

boskos/ranch/storage.go

+	"k8s.io/test-infra/boskos/crds"
 )

 // Storage is used to decouple ranch functionality with the resource persistence layer


this comment is not really accurate anymore.

Do you think so? IMHO it is accurate for as long as we have our internal representation, because the Storage hides the external (CRD) one.

well, I mostly meant that we're no longer depending on the storage.PersistenceLayer interface, and some things (like the runtime client and namespace) are now leaking through.

ixdy · 2020-02-10T23:55:51Z

boskos/ranch/storage.go

+
+	o := &crds.ResourceObject{}
+	name := types.NamespacedName{Namespace: s.namespace, Name: resource.GetName()}
+	if err := s.client.Get(s.ctx, name, o); err != nil {


I assume this bug (where we effectively discard the resource version because we fetch latest and update blindly) will be addressed in a future PR?

Yes, it is an orthogonal change, the overarching goal of this PR is to allow using a cache

ixdy · 2020-02-11T00:00:30Z

boskos/cmd/boskos/boskos.go

 	logrusutil.ComponentInit("boskos")
 	kubeClientOptions.AddFlags(flag.CommandLine)
+	var namespace string
+	flag.StringVar(&namespace, "namespace", corev1.NamespaceDefault, "namespace to install on")


is there a reason you put this in main() instead of with the rest of the flags above?

Oversight, fixed

ixdy · 2020-02-11T00:07:02Z

boskos/crds/client.go

 	}

-	crd := &apiextensionsv1beta1.CustomResourceDefinition{
+	resourceCRD := &apiextensionsv1beta1.CustomResourceDefinition{


idle comment: we should probably update to apiextensions/v1 at some point.

ixdy · 2020-02-11T00:08:12Z

boskos/crds/client.go

 		return err
 	}

-	crd := &apiextensionsv1beta1.CustomResourceDefinition{


we could've kept this generic and looped over the two types, right?

I guess the new version is a little easier to read.

Well yeah, looping would require to have a slice somewhere everyone who could think about adding a CRD will definitely find and I was too lazy to think about the best place for that :)

IMHO we should drop the whole CRD creation entirely, as that requires us to give CRD Create permissions, which shouldn't be needed. I didn't do that in this PR to keep the potential basis for debate as minimal as possible :)

IMHO we should drop the whole CRD creation entirely, as that requires us to give CRD Create permissions, which shouldn't be needed. I didn't do that in this PR to keep the potential basis for debate as minimal as possible :)

I agree. #16266

stevekuznetsov · 2020-02-11T20:11:23Z

boskos/cmd/fake-mason/main.go

 	username          = flag.String("username", "", "Username used to access the Boskos server")
 	passwordFile      = flag.String("password-file", "", "The path to password file used to access the Boskos server")
 	cleanerCount      = flag.Int("cleaner-count", defaultCleanerCount, "Number of threads running cleanup")
+	namespace         = flag.String("namespace", corev1.NamespaceDefault, "namespace to install on")


This precludes a rolling upgrade, right, since we can't add the flag first, then upgrade the version, or the other way around

The flag just got moved, it was part of the kubeOpts before

k8s-ci-robot · 2020-02-11T20:39:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, stevekuznetsov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~boskos/OWNERS~~ [stevekuznetsov]

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

…ctrl-client" This reverts commit 767a53b, reversing changes made to b253986.

alvaroaleman added 3 commits February 10, 2020 11:23

Boskos: Register CRDs via init in default scheme

a782482

List types must be named <<ObjectTypeName>>List

1df14b1

k8s-ci-robot assigned ixdy, sebastienvas and stevekuznetsov Feb 10, 2020

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 10, 2020

k8s-ci-robot requested review from krzyzacy and sebastienvas February 10, 2020 12:43

k8s-ci-robot added area/boskos Issues or PRs related to code in /boskos sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Feb 10, 2020

sebastienvas removed their assignment Feb 10, 2020

ixdy reviewed Feb 11, 2020

View reviewed changes

alvaroaleman added 4 commits February 11, 2020 11:17

Boskos: Ranch: use upstream client

6afc56e

Boskos: Remove downstream client

029e31d

Boskos: Properly set client context

8f3fce5

Boskos: Update bazel

ead8695

alvaroaleman force-pushed the boskos-ctrl-client branch from 5918544 to ead8695 Compare February 11, 2020 10:38

stevekuznetsov reviewed Feb 11, 2020

View reviewed changes

stevekuznetsov approved these changes Feb 11, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 11, 2020

k8s-ci-robot merged commit 767a53b into kubernetes:master Feb 11, 2020

k8s-ci-robot added this to the v1.18 milestone Feb 11, 2020

alvaroaleman added a commit to alvaroaleman/test-infra that referenced this pull request Feb 12, 2020

Revert "Merge pull request kubernetes#16206 from alvaroaleman/boskos-…

87e13af

…ctrl-client" This reverts commit 767a53b, reversing changes made to b253986.

This was referenced Feb 12, 2020

Revert boskos to v20200206-f88edefe8 #16260

Merged

boskos core: don't attempt to create CRDs #16266

Merged

ixdy mentioned this pull request Feb 29, 2020

Update k8s.io/test-infra dependency and fix GCP Mason istio/test-infra#2447

Merged

Comments

Conversation

alvaroaleman commented Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ixdy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Feb 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alvaroaleman commented Feb 10, 2020 •

edited

Loading