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

Pubsub Subscription Support in Prow #10141

Merged
merged 2 commits into from Dec 18, 2018

Conversation

@sebastienvas
Copy link
Collaborator

sebastienvas commented Nov 13, 2018

Design Doc: https://goo.gl/gRsXkD

@k8s-ci-robot k8s-ci-robot requested review from fejta and yutongz Nov 13, 2018

@sebastienvas sebastienvas force-pushed the sebastienvas:pubsub branch 5 times, most recently from c3545ac to d59ad49 Nov 13, 2018

@sebastienvas sebastienvas changed the title [WIP] Pubsub Subscription Support in Prow Pubsub Subscription Support in Prow Nov 17, 2018

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Nov 17, 2018

/cc @krzyzacy

@k8s-ci-robot k8s-ci-robot requested a review from krzyzacy Nov 17, 2018

@krzyzacy

This comment has been minimized.

Copy link
Member

krzyzacy commented Nov 19, 2018

/assign
/woof
will look shortly

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 19, 2018

@krzyzacy: dog image

In response to this:

/assign
/woof
will look shortly

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.

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor

stevekuznetsov commented Nov 27, 2018

Can you group the code to be reviewed in commits and the generated stuff separately? As it is it's a little hard to piece together the different relevant commits you have (squash "simplify code" commit for example)

@sebastienvas sebastienvas force-pushed the sebastienvas:pubsub branch from d59ad49 to 6a3302f Nov 27, 2018

@sebastienvas

This comment has been minimized.

Copy link
Collaborator

sebastienvas commented Nov 27, 2018

@stevekuznetsov please take a look

@stevekuznetsov
Copy link
Contributor

stevekuznetsov left a comment

Changes here look amazing, some small comments

@@ -105,6 +105,9 @@ type ProwConfig struct {
// OwnersDirBlacklist is used to configure which directories to ignore when
// searching for OWNERS{,_ALIAS} files in a repo.
OwnersDirBlacklist OwnersDirBlacklist `json:"owners_dir_blacklist,omitempty"`

// Pub/Sub Subscriptions that we want to listen to
PubsubSubscriptions PubsubSubscriptions `json:"pubsub_subscriptions,omitempty"`

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Nov 27, 2018

Contributor

Bikeshed: PubSub or Pubsub?

This comment has been minimized.

@sebastienvas

sebastienvas Nov 27, 2018

Collaborator

Moved everything to PubSub

@@ -286,6 +289,9 @@ type Branding struct {
HeaderColor string `json:"header_color,omitempty"`
}

// PubsubSubscriptions holds Pubsub subscriptions we want to listen to.

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Nov 27, 2018

Contributor

Can you explain what this maps to what?

This comment has been minimized.

@sebastienvas

sebastienvas Nov 27, 2018

Collaborator

done

Show resolved Hide resolved prow/pubsub/subscriber/server.go
logrus.Warn("New config found, reloading pull Server")
// Making sure the current thread finishes before starting a new one.
errGroup.Wait()
// Starting a new thread with new config

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Nov 27, 2018

Contributor

Why not just pass the workers the ConfigAgent and have them do lookups?

This comment has been minimized.

@sebastienvas

sebastienvas Nov 27, 2018

Collaborator

What are the worker in your idea ? handlePulls ? By the way there is only one worker.
We still need to check if the config changed, and it is just easier to do it in the parent, such that we can cancel the child if a new config exist. Also I prefer passing the config down because from the parent since the parent is already doing a lookup. If the child is doing another lookup the parent and the child might not have the same config, and therefore the parent might issue an extra cancel to the child.

I considered keeping track of all the sub.Receive threads, and removing / adding them when the config is changed, but that s much more complex and prone to more bugs. I could not see the benefits of doing this over just restarting all the threads when a new config is here since pubsub will resend anyway if a message is not acked.

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Nov 28, 2018

Contributor

Hmmm. True, there is only one handlePulls. In one of my WIP pulls I have a config change subscription -- let me post that and we could use one impl.

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Nov 28, 2018

Contributor

I was thinking something like this #10247

This comment has been minimized.

@sebastienvas

sebastienvas Nov 28, 2018

Collaborator

This would help a lot.

"k8s.io/test-infra/prow/kube"

"golang.org/x/sync/errgroup"
"k8s.io/test-infra/prow/pubsub/subscriber"

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Nov 27, 2018

Contributor

nit: imports are not well grouped here

This comment has been minimized.

@sebastienvas

sebastienvas Nov 27, 2018

Collaborator

done

fs.BoolVar(&flagOptions.push, "push", false, "Using Push Server.")
fs.StringVar(&flagOptions.pushSecretFile, "push-secret-file", "", "Path to Pub/Sub Push secret file.")

fs.StringVar(&flagOptions.configPath, "config-path", "../../config.yaml", "Path to config.yaml.")

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Nov 27, 2018

Contributor

There's a refactor incoming to collapse the config parts of the flags but in case that doesn't merge first, can you use the same defaults as everywhere else for common flags?

This comment has been minimized.

@sebastienvas

sebastienvas Nov 27, 2018

Collaborator

yes

flagOptions = &options{}
fs := flag.NewFlagSet(os.Args[0], flag.ExitOnError)
fs.StringVar(&flagOptions.masterURL, "masterurl", "", "URL to k8s master")
fs.StringVar(&flagOptions.kubeConfig, "kubeconfig", "", "Cluster config for the cluster you want to connect to")

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Nov 27, 2018

Contributor

Can we use a common flag util for k8s client?

This comment has been minimized.

@sebastienvas

sebastienvas Nov 27, 2018

Collaborator

yes

logrus.WithError(err).Fatal("Error starting config agent.")
}

// Append the path of hmac and github secrets.

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Nov 27, 2018

Contributor

bad copy pasta?

This comment has been minimized.

@sebastienvas

sebastienvas Nov 27, 2018

Collaborator

yes

@sebastienvas sebastienvas force-pushed the sebastienvas:pubsub branch 5 times, most recently from 43c8f89 to 17e2e1d Nov 27, 2018

@@ -132,61 +125,17 @@ func parseOptions() options {
return o
}

// TODO(krzyzacy): copy & paste, refactor this

This comment has been minimized.

@krzyzacy

krzyzacy Nov 29, 2018

Member

thanks for doing this! 💯

@@ -0,0 +1,87 @@
/*
Copyright 2017 The Kubernetes Authors.

This comment has been minimized.

@krzyzacy

krzyzacy Nov 29, 2018

Member

babynit: 2018, and other places

This comment has been minimized.

@sebastienvas

sebastienvas Dec 17, 2018

Collaborator

Done

@sebastienvas

This comment has been minimized.

Copy link
Collaborator

sebastienvas commented Dec 7, 2018

@krzyzacy please take a look. I fixed your comments, and updated to use the payload to pass in information. I also added an options to pass environment variable to a jobs which will be passed down to all containers (for the release use case since we might point tests to a new artifacts as an example)

limitations under the License.
*/

package kube

This comment has been minimized.

@krzyzacy

krzyzacy Dec 7, 2018

Member

so there's already https://github.com/kubernetes/test-infra/blob/master/prow/flagutil/kubernetes.go, maybe we should merge some logic, or it's time to unify all the different kubeclients we have? @stevekuznetsov

This comment has been minimized.

@sebastienvas

sebastienvas Dec 7, 2018

Collaborator

that's the horrible client that is not compatible with kubeconfig. I think we should kill this.

This comment has been minimized.

@krzyzacy

krzyzacy Dec 7, 2018

Member

yeah, I mean, let everything use this :-)

@krzyzacy

This comment has been minimized.

Copy link
Member

krzyzacy commented Dec 7, 2018

/hold
for @sebastienvas to add some docs

@sebastienvas sebastienvas force-pushed the sebastienvas:pubsub branch from c67d63e to 477562c Dec 7, 2018

@sebastienvas

This comment has been minimized.

Copy link
Collaborator

sebastienvas commented Dec 7, 2018

@krzyzacy documentation added.

@sebastienvas

This comment has been minimized.

Copy link
Collaborator

sebastienvas commented Dec 17, 2018

@krzyzacy please take a look.

@krzyzacy

This comment has been minimized.

Copy link
Member

krzyzacy commented Dec 17, 2018

rebase? :-)

@sebastienvas sebastienvas force-pushed the sebastienvas:pubsub branch from 477562c to 5284d55 Dec 17, 2018

@krzyzacy

This comment has been minimized.

Copy link
Member

krzyzacy commented Dec 18, 2018

/lgtm
squash?

@k8s-ci-robot k8s-ci-robot added the lgtm label Dec 18, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 18, 2018

LGTM label has been added.

Git tree hash: dc69c5544a45c081662aca8e99ae6b808cdac38c

@sebastienvas sebastienvas force-pushed the sebastienvas:pubsub branch from 5284d55 to 844ab99 Dec 18, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Dec 18, 2018

@krzyzacy

This comment has been minimized.

Copy link
Member

krzyzacy commented Dec 18, 2018

/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 18, 2018

LGTM label has been added.

Git tree hash: 4d7d05f76c577eb0e9bfb8a61e439c072a91e705

@k8s-ci-robot k8s-ci-robot added the lgtm label Dec 18, 2018

@sebastienvas

This comment has been minimized.

Copy link
Collaborator

sebastienvas commented Dec 18, 2018

/hold cancel

@sebastienvas

This comment has been minimized.

Copy link
Collaborator

sebastienvas commented Dec 18, 2018

/approve

1 similar comment
@krzyzacy

This comment has been minimized.

Copy link
Member

krzyzacy commented Dec 18, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 18, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: krzyzacy, sebastienvas

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

@k8s-ci-robot k8s-ci-robot merged commit db74ab3 into kubernetes:master Dec 18, 2018

12 checks passed

cla/linuxfoundation sebastienvas authorized
Details
pull-test-infra-bazel Job succeeded.
Details
pull-test-infra-gubernator Skipped
pull-test-infra-lint Job succeeded.
Details
pull-test-infra-verify-bazel Job succeeded.
Details
pull-test-infra-verify-codegen Job succeeded.
Details
pull-test-infra-verify-config Job succeeded.
Details
pull-test-infra-verify-deps Job succeeded.
Details
pull-test-infra-verify-gofmt Job succeeded.
Details
pull-test-infra-verify-govet Job succeeded.
Details
pull-test-infra-verify-labels Skipped
tide In merge pool.
Details
@krzyzacy

This comment has been minimized.

Copy link
Member

krzyzacy commented on prow/pubsub/reporter/reporter.go in bf908c7 Dec 20, 2018

oh wait... @sebastienvas this will break existing jobs :-(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment