Skip to content

[WIP] Node image builds for 3.10 - #1224

Closed
kwoodson wants to merge 4 commits into
openshift:masterfrom
kwoodson:build_node_image
Closed

kwoodson wants to merge 4 commits into
openshift:masterfrom
kwoodson:build_node_image

Conversation

@kwoodson

@kwoodson kwoodson commented Aug 20, 2018

Copy link
Copy Markdown

First attempt at node image build with e2e tests.

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 20, 2018
@kwoodson
kwoodson requested a review from mjudeikis August 20, 2018 19:42
@kwoodson

Copy link
Copy Markdown
Author

Currently failing with az commands not running in containers.

@stevekuznetsov stevekuznetsov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doesn't feel good. We are (ab)using the template mechanism to embed bash into pods and not really making any use of the ci-operator flows from what I can tell.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should be doing this with the tag specification in ci-operator for the 3.10 branch, not here

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed. I'm not sure I understand the tag specification completely. This made it very easy.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mjudeikis

Copy link
Copy Markdown
Contributor

It is still missing ci-operator config part in this PR too.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

s/azure/e2e-azure-secret/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This still needs changing :)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yeah, I was still testing it locally. I thought this was necessary for local tests.

@kwoodson
kwoodson force-pushed the build_node_image branch 5 times, most recently from d284a98 to 3ec8a3a Compare August 31, 2018 21:34

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What about RHEL images?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Today we have separate jobs for the rhel and centos. How do you foresee us having a single template and passing in variables for the build job? I can variable-ize the flavor of OS but how does that get passed into the job?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@Kargakis, any opinion on how this would work?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

delete

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

still outstanding

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

remove

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do you mean loops indefinitely? It shouldn't. What do you see if we dont have timeout?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I experienced failures where the cluster creation would wait indefinitely even though there were failures in the deployment. This occurred when I shared the same resource group with the node build and the cluster build. This pod would never complete so I added a timeout.

@mjudeikis

Copy link
Copy Markdown
Contributor

You might want to update :

master.json	
release-3.10.json
release-3.11.json

to have similar format tag_specification to point to release of origin.

What do you want to do with prow config for this template? we would need still to add it. Separate PR?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is thing we didn't do, but we suppose to. This tags all particular release images into our ci-namespace for us to use. If we need ansible,origin, or any other image from another team - this is how we get them so we dont need to build it ourselves.

@0xmichalis

Copy link
Copy Markdown
Contributor

Please remove the branched jobs, they don't apply to us (at least yet)

@kwoodson

kwoodson commented Sep 5, 2018

Copy link
Copy Markdown
Author

@Kargakis

Please remove the branched jobs, they don't apply to us (at least yet)

What did you mean by this? The release-* files? If that's the case, how do we get 3.10 builds since we need to specify a origin-v3.10 in the release-3.10.yml file under tag_specification?

On a side note, how do we get 3.10 builds for centos and rhel without hard coding them into the template? I put in a variable but I'm unsure how that variable gets populated for the actual run of the job.

@0xmichalis

Copy link
Copy Markdown
Contributor

Your ci-operator jobs should be jobs we run in openshift-ansible and also periodically, right? The openshift-azure and azure-misc release jobs are not used.

@mjudeikis

Copy link
Copy Markdown
Contributor

Your ci-operator jobs should be jobs we run in openshift-ansible and also periodically, right? The openshift-azure and azure-misc release jobs are not used.

we run then in openshift-azure but we consume artifacts from openshift-ansible runs. This way we dont need to rebuild ansible image each time.

@kwoodson

kwoodson commented Sep 5, 2018

Copy link
Copy Markdown
Author
2018/09/05 10:31:01 Running pod dev
2018/09/05 10:56:12 Container buildimage in pod dev completed successfully
2018/09/05 11:13:30 Container buildcluster in pod dev completed successfully
2018/09/05 11:14:22 Container tagimage in pod dev completed successfully
2018/09/05 11:14:59 Container teardown in pod dev completed successfully
2018/09/05 11:14:59 Pod dev succeeded after 43m59s
2018/09/05 11:14:59 Ran for 44m4s

@0xmichalis

Copy link
Copy Markdown
Contributor

we run then in openshift-azure but we consume artifacts from openshift-ansible runs. This way we dont need to rebuild ansible image each time.

Not sure I follow. What's the relation between openshift-azure and the node image? What's the overhead of building the ansible docker image?

@kwoodson

kwoodson commented Sep 5, 2018

Copy link
Copy Markdown
Author

@Kargakis,

The node image is built using the origin-ansible container. Once it has completed, we will use the latest openshift-azure build to test a cluster build. Once the image has passed a cluster build, the image is tagged as valid: true. The teardown is called and the job completes. This job uses both openshift-ansible and openshift-azure to complete its image build and deploy of a cluster.

@mjudeikis

mjudeikis commented Sep 5, 2018 via email

Copy link
Copy Markdown
Contributor

@0xmichalis

Copy link
Copy Markdown
Contributor

The node image is built using the origin-ansible container. Once it has completed, we will use the latest openshift-azure build to test a cluster build. Once the image has passed a cluster build, the image is tagged as valid: true. The teardown is called and the job completes. This job uses both openshift-ansible and openshift-azure to complete its image build and deploy of a cluster.

We have e2e tests for cluster deployments in openshift-azure that use the latest published image. We don't have e2e tests for gating openshift-ansible changes, right? Or at least we need to migrate those jobs away from Jenkins. We should still be able to pull openshift-azure and run a cluster deployment in a job that runs in openshift-ansible.

@kwoodson

kwoodson commented Sep 5, 2018

Copy link
Copy Markdown
Author

@Kargakis

We have e2e tests for cluster deployments in openshift-azure that use the latest published image.

Correct.

We don't have e2e tests for gating openshift-ansible changes, right? Or at least we need to migrate those jobs away from Jenkins.

This is the beginning of that work. The old node image jobs that are in Jenkins do not perform a cluster build. Currently we are not testing our images which is something I was hoping to fix by merging this PR. They used to test cluster builds but we removed that functionality when we stopped using acs-engine deployments.

We should still be able to pull openshift-azure and run a cluster deployment in a job that runs in openshift-ansible.

This is essentially the same thing. Whether we run the job from openshift-azure or openshift-ansible the need for each repo is equivalent. If you prefer moving the job to openshift-ansible and using openshift-azure for images that is fine. Currently the job is using openshift-azure's release-3.10.json and pulling the origin-ansible container image from openshift-ansible. This felt better as we own the process for openshift-azure and we don't have to touch openshift-ansible's files/config/templates.

@0xmichalis

0xmichalis commented Sep 5, 2018 via email

Copy link
Copy Markdown
Contributor

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this is not needed anymore?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

removed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

openshift-azure master is effectively origin 3.10 (we already use 3.10 images in master.yaml). I think this is the wrong place to branch node build jobs, and it's more appropriate to be using the openshift-ansible ci-operator specs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

setup to mirror the install container in the other templates

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

changed to setup

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there any reason for a separate container for doing the tagging? It seems we would reduce the size of bash we maintain by moving this after running the node build playbook?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, just realized you need to tag after the e2e test succeeds. Nvmd.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For ansible PRs and merged commits this needs to be ${LOCAL_IMAGE_BIN}. For periodic jobs, it should be fine as is. Unclear how to best template that.

@0xmichalis

Copy link
Copy Markdown
Contributor

@stevekuznetsov @bbguimaraes any suggestions on how to clone a repo (openshift-azure) in an openshift-ansible test?

@stevekuznetsov

Copy link
Copy Markdown
Contributor

@stevekuznetsov @bbguimaraes any suggestions on how to clone a repo (openshift-azure) in an openshift-ansible test?

What are you trying to acheive? This is the wrong flow.

I think this issue should have been tackled as "how does CI Operator support first-class creation and publishing of artifacts that are not container images or RPMs"

@0xmichalis

0xmichalis commented Sep 13, 2018 via email

Copy link
Copy Markdown
Contributor

@0xmichalis

0xmichalis commented Sep 13, 2018 via email

Copy link
Copy Markdown
Contributor

@stevekuznetsov

Copy link
Copy Markdown
Contributor

As a short-term hack you could use extra_refs in the ProwJob. Please make a card on our JIRA to track publishing non-image artifacts for repos

@0xmichalis

Copy link
Copy Markdown
Contributor

Please make a card on our JIRA to track publishing non-image artifacts for repos

https://jira.coreos.com/browse/DPTP-187

@@ -0,0 +1,315 @@
# This template is being used for testing OpenShift Ansible image builds for Azure.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This template needs to be moved in ci-operator/templates/openshift/openshift-azure.

@0xmichalis

Copy link
Copy Markdown
Contributor

As a short-term hack you could use extra_refs in the ProwJob.

Is this going to work for a periodic job with skip_cloning: true?


cp -r /tmp/shared/_data /go/src/github.com/openshift/openshift-azure/
cd /go/src/github.com/openshift/openshift-azure/
source /etc/azure/credentials/secret

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You also need to source /tmp/shared/environment to get the resource group, right?


# Performs cleanup of all created resources
- name: teardown
image: ${LOCAL_IMAGE_BIN}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LOCAL_IMAGE_SRC should be enough I think

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

BIN and SRC one difference is that BIN has generated code and binaries built already. We use binaries for sync image build in the local namespace. Didn't checked end-to-end but I think we need bin for this, as it is used for sync image build. Just to keep consistent.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But we don't care about building the sync image and we can just pull it from quay since what is tested here is the node build.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess a mismatch between the pushed image in quay and the plugin code that is pulled from git can fail the cluster install, assuming there is a breaking change in between.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

our experience shows we good at this...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably a good one to have so we can stop breaking things :)


# Runs an install
- name: setup
image: ${LOCAL_IMAGE_BIN}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LOCAL_IMAGE_SRC should be enough I think

export DNS_DOMAIN=osadev.cloud
export DNS_RESOURCEGROUP=dns
export DEPLOY_VERSION=v${OPENSHIFT_RELEASE}
export RUN_SYNC_LOCAL=true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

remove


source /etc/azure/credentials/secret
az login --service-principal -u ${AZURE_CLIENT_ID} -p ${AZURE_CLIENT_SECRET} --tenant ${AZURE_TENANT_ID} &>/dev/null
# TODO: Configure these from prow

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

remove

@0xmichalis

Copy link
Copy Markdown
Contributor

The periodic prowjob I came up with:

  - agent: kubernetes
    decorate: true
    name: azure-build-node-image-rhel-310
    extra_refs:
    - org: openshift
      repo: openshift-azure
      base_ref: master
    skip_cloning: true
    spec:
      containers:
      - args:
        - --secret-dir=/usr/local/e2e-azure-secret
        - --template=/usr/local/azure-image-build
        command:
        - ci-operator
        env:
        - name: CONFIG_SPEC
          valueFrom:
            configMapKeyRef:
              key: release-3.10.yaml
              # Use openshift-ansible ci-operator config for as long as
              # the node build code is part of that repo.
              name: ci-operator-openshift-openshift-ansible
        - name: JOB_NAME_SAFE
          value: azure-image-build
        - name: CLUSTER_TYPE
          value: azure
        image: ci-operator:latest
        name: test
        volumeMounts:
        - mountPath: /usr/local/azure-image-build
          name: job-definition
          subPath: azure-image-build.yaml
        - mountPath: /usr/local/e2e-azure-secret
          name: cluster-secrets-azure-file
      serviceAccountName: ci-operator
      volumes:
      - configMap:
          name: prow-job-azure-build-image
        name: job-definition
      - name: cluster-secrets-azure-file
        projected:
          sources:
          - secret:
              name: cluster-secrets-azure-file

Haven't actually tested but my question is, will LOCAL_IMAGE_SRC include the source code from extra_refs? @bbguimaraes @stevekuznetsov @petr-muller If so, @kwoodson I think we can get rid of the release-3.10.yaml ci-operator config that you are adding in this PR and use the openshift-ansible ci-operator config directly.

@mjudeikis

Copy link
Copy Markdown
Contributor

I think we can get rid of the release-3.10.yaml ci-operator config that you are adding in this PR and use the openshift-ansible ci-operator config directly.

I think I know what you have in mind, but need more tech details on how it should be implemented.

@0xmichalis

Copy link
Copy Markdown
Contributor

Closing in favor of #1502

@0xmichalis 0xmichalis closed this Sep 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants