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.

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

add pull-k8sio-backup presubmit check #15398

Merged
merged 1 commit into from
Jan 2, 2020

Conversation

listx
Copy link
Contributor

@listx listx commented Nov 26, 2019

This checks the backup_tools scripts by testing them. Specifically, it
runs the backup_test.sh script, which performs the backup from

us.gcr.io/k8s-gcr-backup-test-prod

to

us.gcr.io/k8s-gcr-backup-test-prod-bak

for a set of chosen images. Both this test and the real backup logic in
backup_prod.sh share the same "build_gcrane" and "copy_with_date"
functions --- the main difference is the repositories where the backups
happen and the images that are backed up.

NOTE: I need to provide the test-environment secret k8s-gcr-backup-test-prod-bak-service-account (i.e., k8s-infra-gcr-promoter@k8s-gcr-backup-test-prod-bak.iam.gserviceaccount.com) for this job to work.

/hold

/cc @thockin @dims @justinsb

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/config Issues or PRs related to code in /config area/release-eng Issues or PRs related to the Release Engineering subproject sig/release Categorizes an issue or PR as relevant to SIG Release. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Nov 26, 2019
@listx listx force-pushed the backup-presubmit-test branch from 12a8e65 to e99f047 Compare November 26, 2019 03:01
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 26, 2019
@listx
Copy link
Contributor Author

listx commented Dec 3, 2019

/assign @cjwagner

@listx
Copy link
Contributor Author

listx commented Dec 3, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 3, 2019
@listx
Copy link
Contributor Author

listx commented Dec 3, 2019

Oops, forgot that I need to provide the secret for this job to work.

Re-holding...

/hold

Assigning to test-infra oncall:
/assign @cjwagner

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 3, 2019
volumes:
- name: creds
secret:
secretName: k8s-gcr-backup-test-prod-bak-service-account
Copy link
Member

Choose a reason for hiding this comment

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

Is it safe to have these credentials used in a presubmit and hosted on the untrusted build cluster? You should assume that any presubmit job can mount and use credentials in that cluster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes --- the cred will only ever have write access to the ephemeral test resources an GCP.

# Check that changes to backup scripts are valid.
- name: pull-k8sio-backup
decorate: true
skip_report: false
Copy link
Member

Choose a reason for hiding this comment

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

This is the default, you can omit it.

- name: pull-k8sio-backup
decorate: true
skip_report: false
run_if_changed: 'infra/gcp/backup_tools/.*'
Copy link
Member

Choose a reason for hiding this comment

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

nit: ^infra/gcp/backup_tools/ probably more closely matches what you want.

- image: gcr.io/k8s-testimages/kubekins-e2e:v20191124-4beb966-1.17
command:
- infra/gcp/backup_tools/backup_test.sh
env:
Copy link
Member

Choose a reason for hiding this comment

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

I think your indentation is off starting on this line.

Copy link
Member

Choose a reason for hiding this comment

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

The other comments can be ignored, but I'm pretty sure this won't work as you expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the record, I suppose it's troublesome that the presubmit checks passed regardless.

Copy link
Member

Choose a reason for hiding this comment

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

It is a known issue, but I haven't addressed it out of fear that strictly parsing pod specs will fail if new fields that Prow doesn't know about are used. It may not be a legitimate concern though, it is kind of hard for me to tell if that would actually be a common problem. It may be fine if

  • new podspec fields aren't used much in prowjobs
  • we keep this dependency up to date and continue to keep prow.k8s.io up to date:

    test-infra/repos.bzl

    Lines 1448 to 1456 in 44c22ec

    go_repository(
    name = "io_k8s_apimachinery",
    build_file_generation = "on",
    build_file_proto_mode = "disable",
    importpath = "k8s.io/apimachinery",
    replace = "k8s.io/apimachinery",
    sum = "h1:7Kns6qqhMAQWvGkxYOLSLRZ5hJO0/5pcE5lPGP2fxUw=",
    version = "v0.0.0-20190817020851-f2f3a405f61d",
    )

This checks the backup_tools scripts by testing them. Specifically, it
runs the backup_test.sh script, which performs the backup from

    us.gcr.io/k8s-gcr-backup-test-prod

    to

    us.gcr.io/k8s-gcr-backup-test-prod-bak

for a set of chosen images. Both this test and the real backup logic in
backup_prod.sh share the same "build_gcrane" and "copy_with_date"
functions --- the main difference is the repositories where the backups
happen and the images that are backed up.
@listx listx force-pushed the backup-presubmit-test branch from e99f047 to 85fcf2a Compare December 12, 2019 06:12
@listx
Copy link
Contributor Author

listx commented Dec 12, 2019

@cjwagner PTAL

@listx listx requested a review from cjwagner December 12, 2019 06:16
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 17, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjwagner, listx

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

@listx
Copy link
Contributor Author

listx commented Jan 2, 2020

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 2, 2020
@k8s-ci-robot k8s-ci-robot merged commit f7f462d into kubernetes:master Jan 2, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Jan 2, 2020
@k8s-ci-robot
Copy link
Contributor

@listx: Updated the job-config configmap in namespace default at cluster default using the following files:

  • key container-image-promoter.yaml using file config/jobs/kubernetes/sig-release/cip/container-image-promoter.yaml

In response to this:

This checks the backup_tools scripts by testing them. Specifically, it
runs the backup_test.sh script, which performs the backup from

us.gcr.io/k8s-gcr-backup-test-prod

to

us.gcr.io/k8s-gcr-backup-test-prod-bak

for a set of chosen images. Both this test and the real backup logic in
backup_prod.sh share the same "build_gcrane" and "copy_with_date"
functions --- the main difference is the repositories where the backups
happen and the images that are backed up.

NOTE: I need to provide the test-environment secret k8s-gcr-backup-test-prod-bak-service-account (i.e., k8s-infra-gcr-promoter@k8s-gcr-backup-test-prod-bak.iam.gserviceaccount.com) for this job to work.

/hold

/cc @thockin @dims @justinsb

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/config Issues or PRs related to code in /config area/release-eng Issues or PRs related to the Release Engineering subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants