Set Up Publishing-Bot #520

ameukam · 2019-12-17T23:54:43Z

Part of #503.

Configs and deployments files are from Publishing-Bot repo.

/assign @dims @nikhita

publishing-bot/configs/kubernetes-nightly

publishing-bot/manifests/secret.yaml

publishing-bot/Makefile

publishing-bot/configs/kubernetes-nightly

publishing-bot/manifests/secret.yaml

publishing-bot/configs/example

publishing-bot/configs/kubernetes-nightly-rules-configmap.yaml

publishing-bot/configs/kubernetes-rules-configmap.yaml

publishing-bot/manifests/deployment.yaml

nikhita · 2020-01-20T12:10:31Z

@ameukam can you also add validate, build-image, etc in the Makefile as in https://github.com/kubernetes/publishing-bot/blob/master/Makefile? We'll need that to build the image before deploying...

publishing-bot/Makefile

bartsmykla · 2020-01-22T13:41:50Z

/lgtm

publishing-bot/Makefile

k8s-ci-robot · 2020-01-24T17:43:03Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign bartsmykla
You can assign the PR to them by writing /assign @bartsmykla in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

bartsmykla · 2020-02-04T09:34:56Z

/lgtm

thockin · 2020-02-13T21:50:24Z

publishing-bot/README.md

+and releases >= v1.14, we run two instances of the bot today.
+
+The instance of the bot responsible for syncing releases <= v1.14 runs in the
+`k8s-publishing-bot-godeps` namespace and the instance of the bot responsible


This NS does not exist yet - should this PR add it to infra/gcp/namespaces/ensure-namespaces.sh ?

publishing-bot/configs/kubernetes-configmap.yaml

thockin · 2020-02-13T21:54:52Z

publishing-bot/configs/kubernetes-nightly-configmap.yaml

+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: publisher-config


Would it make sense to have 2 subdirs, "kubernetes" and "kubernetes-nightly" to isolate the files better? would make diffing them easier, too

thockin · 2020-02-13T21:57:43Z

publishing-bot/manifests/podspec.yaml

+      port: 80
+  resources:
+    requests:
+      cpu: 300m


why is CPU hardcoded but memory is parameterized?

also, .3 cores is a LOT of CPU

We have client-go smoke tests in there, which require a certain amount of CPU in order to be stable.

thockin · 2020-02-13T21:58:11Z

publishing-bot/manifests/podspec.yaml

+      cpu: 300m
+      memory: MEMORY_REQUESTS
+    limits:
+      cpu: 2


don't set CPU limits. There's no point to it.

thockin · 2020-02-13T22:04:20Z

publishing-bot/manifests/rs.yaml

@@ -0,0 +1,15 @@
+apiVersion: apps/v1
+kind: Replicaset


Why is this not using Deployment?

@nikhita asked to keep it as Replicaset in a previous conversation: #520 (comment)

I don't see why this is needed, though. Is it JUSt to avoid changing 2 things at once?

thockin · 2020-02-13T22:08:39Z

publishing-bot/manifests/storage-class.yaml

@@ -0,0 +1,7 @@
+kind: StorageClass


This should go ... somewhere else. Probably in clusters/kubernetes-public/aaa - see https://www.terraform.io/docs/providers/kubernetes/r/storage_class.html

This could be a PR of its own

thockin · 2020-02-13T22:10:08Z

publishing-bot/Makefile

+
+SHELL := /bin/bash
+
+build:


This doesn't seem to apply at all in this directory? The code is not here

thockin · 2020-02-13T22:11:17Z

publishing-bot/Makefile

+.PHONY: validate
+
+init-deploy: validate
+	$(KUBECTL) delete -n "$(NAMESPACE)" --ignore-not-found=true replicaset publisher


This feels a little baroque - why are we trying to delete things?

thockin · 2020-02-13T22:14:34Z

publishing-bot/Makefile

+	{ cat manifests/pod.yaml && sed 's/^/  /' manifests/podspec.yaml; } | \
+	$(call prepare_spec) | $(KUBECTL) apply -n "$(NAMESPACE)" -f -
+
+deploy: init-deploy


Document assumptions - like the current context

sttts · 2020-02-14T08:19:18Z

publishing-bot/Makefile

+	$(KUBECTL) apply -n "$(NAMESPACE)" -f manifests/service.yaml
+	{ cat manifests/rs.yaml && sed 's/^/      /' manifests/podspec.yaml; } | \
+	$(call prepare_spec) | sed 's/-interval=0/-interval=$(INTERVAL)/g' | \
+	$(KUBECTL) apply -n "$(NAMESPACE)" -f -


why are we duplicating deployment code from the publishing bot? 😱 This repo is invisible to anybody doing changes in the bot repository, which will eventually break this copy.

With the migration of the publishing bot to a new community-owned GKE cluster (aaa), we want to migrate some parts of the deployment code. What's the right approach in this scenario ?

This code in the bot repo is used by a number of parties. So we won't remove it. Hence, you are creating a copy here which will most certainly divert.

Where is the problem of keeping the deployment code where it is? The people deploying the publishing bot (which is @nikhita, @dims and myself only; nobody without deep knowledge of what the bot does should do this) all are used to the bot repo. Which pain points should this duplication solve?

I would also be fine to do some git clone magic here checking out the bot repo and call the Makefile in there. Then it's well documented what is going on.

I am fine to leave the config in the bot repo, I guess. This repo/dir could be reduced to a README explaining how we deploy it in "aaa"

But all my comments here apply - especially around CPu and memory limits.

@ameukam can you move the fixes to the publishing-bot repo instead? @nikhita and I are happy to review over there. Thanks!

@sttts @thockin @ameukam @nikhita I have added to todays agenda for k8s-infra-wg bi-weekly meeting discussion about this topic. Please attend if you want to discuss about it in syncronous manner. http://bit.ly/wg-k8s-infra-notes

bartsmykla · 2020-02-24T09:10:44Z

@ameukam what do you think about @sttts suggestion about moving the fixes to publishing-bot repo? Can I help you with anything to move this thing forward? :-)

ameukam · 2020-02-24T09:24:39Z

@bartsmykla I'm going to work on it this week.

bartsmykla · 2020-02-24T09:28:05Z

@ameukam perfect! If you'll need any help just ping me on slack :-)

bartsmykla · 2020-03-06T07:38:03Z

@ameukam is there anything I can help you with to move this forward? :-)

sttts · 2020-04-01T17:30:37Z

/hold

Just to be sure it is not merged accidently until the comments are addresses that we don't want to duplicate build code for the bot.

bartsmykla · 2020-04-10T08:27:21Z

@ameukam can you give us some updates about the state of the issue? I can help with moving some of the resources like StorageClass to different places like @thockin suggested. If you don't have time I can also do more work to move the things forward. :-)

ameukam · 2020-04-10T08:32:29Z

@bartsmykla kubernetes/publishing-bot#221 got merged. I think this PR is not useful anymore. I will close this PR when publishing-bot will be migrated.

nikhita · 2020-04-23T08:24:01Z

@ameukam Even though we ended up taking a different approach, thank you SO MUCH for taking the time to work on this and driving the migration! ❤️

Going to close this since migration is now complete (#503).
/close

k8s-ci-robot · 2020-04-23T08:24:16Z

@nikhita: Closed this PR.

In response to this:

@ameukam Even though we ended up taking a different approach, thank you SO MUCH for taking the time to work on this and driving the migration! ❤️

Going to close this since migration is now complete (#503).
/close

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.

bartsmykla · 2020-04-23T08:26:19Z

Huge thanks for @ameukam from me too!

bartsmykla · 2020-04-23T08:32:54Z

And to you @nikhita too! :-)

k8s-ci-robot assigned dims Dec 17, 2019

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 17, 2019

k8s-ci-robot assigned nikhita Dec 17, 2019

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. wg/k8s-infra labels Dec 17, 2019

k8s-ci-robot requested review from bartsmykla and nikhita December 17, 2019 23:55

ameukam commented Dec 18, 2019

View reviewed changes

publishing-bot/configs/kubernetes-nightly Show resolved Hide resolved

ameukam commented Dec 18, 2019

View reviewed changes

publishing-bot/manifests/secret.yaml Outdated Show resolved Hide resolved

nikhita reviewed Jan 20, 2020

View reviewed changes

publishing-bot/Makefile Outdated Show resolved Hide resolved

publishing-bot/configs/kubernetes-nightly Show resolved Hide resolved

publishing-bot/manifests/secret.yaml Outdated Show resolved Hide resolved

publishing-bot/configs/example Outdated Show resolved Hide resolved

bartsmykla reviewed Jan 20, 2020

View reviewed changes

nikhita reviewed Jan 20, 2020

View reviewed changes

publishing-bot/Makefile Outdated Show resolved Hide resolved

ameukam changed the title ~~WIP: Set Up Publishing-Bot~~ Set Up Publishing-Bot Jan 21, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 21, 2020

k8s-ci-robot assigned bartsmykla Jan 22, 2020

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

nikhita reviewed Jan 22, 2020

View reviewed changes

publishing-bot/Makefile Outdated Show resolved Hide resolved

Set Up Publishing-Bot

2f12f45

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 24, 2020

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

thockin reviewed Feb 13, 2020

View reviewed changes

sttts reviewed Feb 14, 2020

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 1, 2020

ameukam mentioned this pull request Apr 2, 2020

Prepare publishing-bot migration kubernetes/publishing-bot#221

Merged

k8s-ci-robot closed this Apr 23, 2020


		SHELL := /bin/bash

		build:

Set Up Publishing-Bot #520

Set Up Publishing-Bot #520

Conversation

ameukam commented Dec 17, 2019

nikhita commented Jan 20, 2020

bartsmykla commented Jan 22, 2020

k8s-ci-robot commented Jan 24, 2020

bartsmykla commented Feb 4, 2020

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

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

sttts Feb 14, 2020 • edited Loading

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

bartsmykla commented Feb 24, 2020

ameukam commented Feb 24, 2020

bartsmykla commented Feb 24, 2020

bartsmykla commented Mar 6, 2020

sttts commented Apr 1, 2020

bartsmykla commented Apr 10, 2020

ameukam commented Apr 10, 2020

nikhita commented Apr 23, 2020

k8s-ci-robot commented Apr 23, 2020

bartsmykla commented Apr 23, 2020

bartsmykla commented Apr 23, 2020

sttts Feb 14, 2020 •

edited

Loading