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

ci-operator: stop allowing users to set artifact dir #1731

Conversation

stevekuznetsov
Copy link
Contributor

No longer are users able to set the explicit directory for:

  • ci-operator outputs
  • simple Pod tests
  • multi-stage test steps

ci-operator itself will read the ${ARTIFACTS} value to determine
where to put its own artifacts (like the output jUnit files). This is in
effect what already happens, as this is the fallback to when the flag is
not passed, and we do not pass it in generated jobs. The flag will
remain in a deprecated manner so as to not break users.

All test steps except for Template tests are already using the
test-infra pod-utils to upload, and therefore are not allowed to set a
non-standard directory, anyway. Nothing changes for these steps. It is
now not possible to opt out of this by telling ci-operator not to use
pod-utils.

We continue to use the deprecated artifact fetching sidecar code for
Templates, but only for templates.

This will cause a breaking change to prowgen as our previous automation
explicitly wrote our defaulted value into all configs even if they never
set it.

Signed-off-by: Steve Kuznetsov skuznets@redhat.com

/assign @alvaroaleman @petr-muller @bbguimaraes

@stevekuznetsov
Copy link
Contributor Author

stevekuznetsov commented Feb 25, 2021

Part of DPTP-1668

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 25, 2021
@@ -366,27 +367,6 @@ func addPodUtils(pod *coreapi.Pod, artifactDir string, decorationConfig *prowv1.
return nil
}

func addArtifacts(pod *coreapi.Pod, artifactDir string) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is suspicious but I think templates don't actually use this, they use addArtifactsToPod ...

@stevekuznetsov
Copy link
Contributor Author

Hmm I think I can split this up

@stevekuznetsov
Copy link
Contributor Author

OK, so it theoretically should be possible to split this into steps:

  1. remove the feature from basic container jobs
  2. remove it from step registry stuff
  3. remove it from the ci-operator top-level

I'll try to do that - there are areas where we infer 1&2 from 3 randomly so it's a bit messy but probably doable.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 1, 2021
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 1, 2021
@stevekuznetsov
Copy link
Contributor Author

@alvaroaleman @petr-muller updated this to be minimal now that the rest of the changes have come in. Notably, this removes the --artifact-dir setting from ci-operator itself. I think this is a useful thing to do as:

  1. we never set it explicitly
  2. we only care about using it when Prow's running us

In openshift/release today, we have this:

$ git grep -h artifact-dir | head -n 1
        - --artifact-dir=$(ARTIFACTS)
$ git grep artifact-dir | wc -l
563
$ git grep artifact-dir | grep -v ARTIFACTS | wc -l
0

So there's 563 places where we set it to what would otherwise be the default value anyway, so there's zero utility to this flag. In addition, having to pass this directory around everywhere in our codebase ended up being a bad cargo-cult ... half the places didn't even end up using it, and in some cases like multi-stage it looks like we accidentally used this instead of the per-step one ...

images/entrypoint-wrapper/Dockerfile Outdated Show resolved Hide resolved
pkg/util/env.go Outdated Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2021
@stevekuznetsov stevekuznetsov force-pushed the skuznets/remove-artifact-dir branch 2 times, most recently from bd055de to e77c8c4 Compare March 3, 2021 17:12
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2021
@stevekuznetsov
Copy link
Contributor Author

/retest

cmd/ci-operator/main.go Show resolved Hide resolved
cmd/ci-operator/main.go Outdated Show resolved Hide resolved
images/entrypoint-wrapper/Dockerfile Outdated Show resolved Hide resolved
@stevekuznetsov
Copy link
Contributor Author

@alvaroaleman updated

@stevekuznetsov
Copy link
Contributor Author

Seeing a lot of this lately

 error: error creating buildah builder: Error writing blob: error storing blob to file "/var/tmp/storage350157992/5": unexpected EOF 

/retest

No longer are users able to set the explicit directory for:
 - `ci-operator` outputs
 - simple Pod tests
 - multi-stage test steps

`ci-operator` itself will read the `${ARTIFACTS}` value to determine
where to put its own artifacts (like the output jUnit files). This is in
effect what already happens, as this is the fallback to when the flag is
not passed, and we do not pass it in generated jobs. The flag will
remain in a deprecated manner so as to not break users.

All test steps *except* for `Template` tests are already using the
test-infra pod-utils to upload, and therefore are not allowed to set a
non-standard directory, anyway. Nothing changes for these steps. It is
now not possible to opt out of this by telling `ci-operator` not to use
pod-utils.

We continue to use the deprecated artifact fetching sidecar code for
Templates, but only for templates.

This will cause a breaking change to prowgen as our previous automation
explicitly wrote our defaulted value into all configs even if they never
set it.

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
@stevekuznetsov
Copy link
Contributor Author

Rebased and ready @alvaroaleman @petr-muller

Copy link
Member

@petr-muller petr-muller left a comment

Choose a reason for hiding this comment

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

LGTM

/hold
Holding to allow resolving Alvaro's comment

cmd/ci-operator/main.go Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Mar 5, 2021
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: petr-muller, stevekuznetsov

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:
  • OWNERS [petr-muller,stevekuznetsov]

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

@stevekuznetsov
Copy link
Contributor Author

/hold cancel

You need to opt in via flag

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 5, 2021
@openshift-merge-robot openshift-merge-robot merged commit 08d4c99 into openshift:master Mar 5, 2021
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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants