Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Change Boskos StatefulSet to Deployment. #225

Merged
merged 1 commit into from
Sep 15, 2020
Merged

Change Boskos StatefulSet to Deployment. #225

merged 1 commit into from
Sep 15, 2020

Conversation

tomgeorge
Copy link
Contributor

Changes

This change deploys Boskos as a Deployment rather than a StatefulSet, to enable quicker feedback in the deployment process, and removing the need to delete the Boskos pods before new pods are started.

This addresses #193

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide
for more details.

@tekton-robot
Copy link
Contributor

Hi @tomgeorge. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@tekton-robot tekton-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 9, 2020
@vdemeester
Copy link
Member

/ok-to-test

@tekton-robot tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 10, 2020
afrittoli
afrittoli reviewed Feb 10, 2020
View reviewed changes
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this!
To be honest I don't fully understand the ramifications of dropping the PVC here, but since this change replicates what was done on the infra side, I think it's good.
/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 10, 2020
@tomgeorge
Copy link
Contributor Author

Sorry for the delay. I wasn't super sure either, I had faith that test-infra did the right thing 😅

Looking into it a little more, I think Boskos keeps a list of resources that are stored in the PVC. See https://github.com/kubernetes/test-infra/blob/2ec0aac72450a8617baf78066a08bb7e943289ee/boskos/common/common.go#L103 for the definition of the common.Resource struct.

The storage plugin for boskos adds/removes those Resources to the PVC.

Boskos added the ability to store that user data as CRDs, so I think going forward they have migrated to that. The Resource information (Name, Type, Last Updated, etc) are stored in the API server.

@tomgeorge
Copy link
Contributor Author

To clarify, it's storing the current state of Boskos and the resources under management

@ghost
Copy link

ghost commented Jul 17, 2020

/hold

/approve

@tomgeorge @afrittoli @bobcatfish do we want to move ahead on this issue & PR? Any outstanding questions or problems before we merge?

@tekton-robot tekton-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 17, 2020
@bobcatfish bobcatfish mentioned this pull request Aug 13, 2020
Closed
@tekton-robot
Copy link
Contributor

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

@tekton-robot
Copy link
Contributor

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

Send feedback to tektoncd/plumbing.

@tekton-robot tekton-robot added the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 16, 2020
@tekton-robot
Copy link
Contributor

@tekton-robot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

Send feedback to tektoncd/plumbing.

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.

@dibyom dibyom mentioned this pull request Sep 11, 2020
Closed
@dibyom dibyom reopened this Sep 11, 2020
@dibyom
Copy link
Member

dibyom commented Sep 11, 2020 •
edited
Loading

From my investigation in #566, I think we should be able to move to a Deployment from a Stateful set with no issues.

Specifically: #566 (comment)

@dibyom dibyom removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Sep 11, 2020
dibyom
dibyom reviewed Sep 11, 2020
View reviewed changes
boskos/boskos.yaml Outdated
@@ -91,31 +73,19 @@ spec:
- name: boskos
image: gcr.io/k8s-prow/boskos/boskos:v20200117-6384054e3
args:
- --storage=/store/boskos.json
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would have prevented #566

@dibyom
Copy link
Member

dibyom commented Sep 11, 2020

/kind cleanup

@tekton-robot tekton-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Sep 11, 2020
@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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

@ghost
Copy link

ghost commented Sep 14, 2020

@tomgeorge feel free to cancel the hold if you want to proceed with this PR!

@tomgeorge
Copy link
Contributor Author

/hold cancel

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 14, 2020
@dibyom
Copy link
Member

dibyom commented Sep 14, 2020

/test plumbing-yamllint

@tomgeorge
Change Boskos StatefulSet to Deployment.
172c14c 
This change deploys Boskos as a Deployment rather than a StatefulSet, to enable quicker feedback in the deployment process, and removing
the need to delete the Boskos pods before new pods are started.

This addresses tektoncd#193

Signed-off-by: Tom George <tg82490@gmail.com>
@tomgeorge tomgeorge force-pushed the boskos-statefulset-to-deployment branch from cb2416d to 172c14c Compare September 14, 2020 20:57
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 14, 2020
@tomgeorge
Copy link
Contributor Author

tomgeorge commented Sep 14, 2020 •
edited
Loading

rebased master onto boskos-statefulset-to-deployment

@dibyom @sbwsg can you take a look and re-add the LGTM

@vdemeester
Copy link
Member

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 15, 2020
@tekton-robot tekton-robot merged commit 4cc9101 into tektoncd:master Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afrittoli afrittoli afrittoli left review comments

@dibyom dibyom dibyom left review comments

@dlorenc dlorenc Awaiting requested review from dlorenc

@vdemeester vdemeester Awaiting requested review from vdemeester

Assignees

@vdemeester vdemeester

@afrittoli afrittoli

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tomgeorge @tekton-robot @vdemeester @dibyom @afrittoli