Skip to content
This repository has been archived by the owner on Jun 14, 2019. It is now read-only.

prowgen multiple targets #79

Open
mjudeikis opened this issue Feb 15, 2019 · 9 comments
Open

prowgen multiple targets #79

mjudeikis opened this issue Feb 15, 2019 · 9 comments

Comments

@mjudeikis
Copy link

Our jobs depends on multiple targets.
In example running job X it requires multiple targets to be ran for it:

    name: pull-ci-openshift-openshift-azure-master-e2e-upgrade-v1.2
    rerun_command: /test e2e-upgrade-v1.2
    spec:
      containers:
      - args:
        - --artifact-dir=$(ARTIFACTS)
        - --give-pr-author-access-to-namespace=true
        - --target=[images]
        - --target=[output:stable:azure-controllers]
        - --target=[output:stable:etcdbackup]
        - --target=[output:stable:metricsbridge]
        - --target=[output:stable:sync]
        - --target=rbac
        - --target=e2e-upgrade-v1.2

In this case e2e-upgrade-v1.2 requires all images to be available, and template rbac to be available too. But prowgen when generating step for:

- artifact_dir: /tmp/artifacts
  as: e2e-upgrade-v1.2
  commands: ARTIFACT_DIR=/tmp/artifacts SOURCE=v1.2 make upgrade
  secret: 
    name: azure
    mount_path: /usr/secrets
  container:
    from: src

Does not allow to all additional targets :/

@petr-muller
Copy link
Member

As discussed on Slack, "jobs that require multiple targets" should not be a thing. If there is a dependency between targets, ci-operator should be able to compute it from its dependency graph - that's one of its raisons d'etre. Prowgen follows ci-operator concepts. The right place to fix this is to make ci-operator aware of this dependency.

@mjudeikis
Copy link
Author

I'm having a hard time imagining how ci-operator can be made aware of this :/

@mjudeikis
Copy link
Author

I can currently add these jobs and ci-operator can deal with it. But I cant commit files because prowgen cleans the targets. So it looks like prowgen issue to me.

@petr-muller
Copy link
Member

petr-muller commented Feb 15, 2019

You could create a job with multiple targets easily - just hand-craft a job that is not generated and Prowgen (or better, its openshift/release check) won't give a damn as long as it does not have a name identical to something Prowgen would generate. Nothing prevents you from having such job. The problem is that our automation forces you to always have that single-target job too. This follows the assumption that ci-operator can resolve dependencies between targets. If it cannot, then it needs to be fixed. If the assumption does not hold, we'll need to revamp the thole system, but I would prefer to just fix ci-operator.

@mjudeikis
Copy link
Author

@petr-muller im happy to do this, but I still need a way to pass default generated jobs via rehearse jobs:
like this one: openshift/release#2883
ci/rehearse/openshift/openshift-azure/master/e2e-upgrade-v2.0

@petr-muller
Copy link
Member

@mjudeikis The issue where rehearsal jobs were blocking in openshift/release should be fixed. I think openshift/release#2883 should be now merged on /lgtm

@mjudeikis
Copy link
Author

Thanks
Do we want to do something with this multitarget stuff or close it?

@mjudeikis
Copy link
Author

We just did this, but now our job config file is a mess... we have /test e2e-upgrade-v1.2 which are prowgen generated one and will never work because of missing targets, and we have /test upgrade-v1.2 which is manually constructed and the one we gonna use. It would be good to fix decide how we wanna fix job where multiple targets are needed.

@petr-muller
Copy link
Member

I believe we need to improve ci-operator to be able to compute the dependencies of targets so that the user never needs to provide more that one target to make a target actually pass. Either ci-operator should compute it automatically (if possible), or we need to provide some additional "target X depends on target Y" syntax to config files.

Can you please file an issue on https://github.com/openshift/ci-operator, describe there how your targets are dependent and why, and maybe link these related issues there so we can record the context? Then we can close this one.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants