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

HACKING.md: add docs for iterating without building images #189

Merged
merged 1 commit into from
Dec 11, 2018

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Nov 21, 2018

No description provided.

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 21, 2018
@cgwalters
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 21, 2018
@cgwalters
Copy link
Member

Then, one can simply scp newly built binaries to /usr/local/bin on all the nodes

Doing this required a two-step process by default since the core user can't write to /usr/local/bin. Did you also hack this?

Offhand I think though it'd combine the advantages of both to have a flow that just copied the built binary in the worktree on top of the existing container using unpriv podman, and then pushed it into the cluster's registry and updated the ds?

@jlebon
Copy link
Member Author

jlebon commented Nov 21, 2018

Ahh yeah, one of my first steps after setting up a cluster is:

for ip in 10 11 51; do tmpssh core@192.168.126.$ip sudo cp -R /home/core/.ssh /root; done

Offhand I think though it'd combine the advantages of both to have a flow that just copied the built binary in the worktree on top of the existing container using unpriv podman, and then pushed it into the cluster's registry and updated the ds?

Yeah, that'd probably be cleaner since it'd avoid having to kill the MCO.

@kikisdeliveryservice
Copy link
Contributor

kikisdeliveryservice commented Nov 21, 2018

Just following this PR bc it would make my life easier 👍

@cgwalters
Copy link
Member

OK I started on this today:

#!/usr/bin/env bash

# Overlay the locally-built binary on top of the existing production
# image, and push it to the cluster registry, and change the deployment
# to reference it.

# Assumptions: You have set KUBECONFIG to point to your local cluster.
# Currently assumes you are running a proxy locally to the cluster's registry
# service; e.g. I did `oc get -n openshift-image-registry svc` to get the Cluster IP,
# then e.g.: ssh -N -L 5000:10.3.138.149:5000 core@192.168.126.11

set -xeuo pipefail

oc get -n openshift-cluster-version deploy
if oc get -n openshift-cluster-version deploy/cluster-version-operator 2>/dev/null; then
    echo "Killing cluster-version-operator"
    oc delete -n openshift-cluster-version deploy/cluster-version-operator
fi

WHAT=machine-config-daemon
LOCALNAME=localhost/${USER}/${WHAT}
#podman build -t "${LOCALNAME}" -f Dockerfile.${WHAT}
podman push --tls-verify=false "${LOCALNAME}" localhost:5000/default/${WHAT}

oc patch -n openshift-machine-config-operator ds/machine-config-operator -p '{"spec": {"template": {"spec": {"containers"}}}}'

Going to work on it some more tomorrow.

@cgwalters
Copy link
Member

OK so this ssh proxy thing is a hack and isn't working for me now I think due to needing to be in the container network, not the host. I guess what we really want here is a router for libvirt?

@cgwalters
Copy link
Member

OK so oc proxy seems like it'd help but podman push expects registries to not have URL components, so we'd need another proxy in there?

@cgwalters
Copy link
Member

Current script:

#!/usr/bin/env bash

# Overlay the locally-built binary on top of the existing production
# image, and push it to the cluster registry, and change the deployment
# to reference it.

# Assumptions: You have set KUBECONFIG to point to your local cluster.
# You must run e.g. `oc proxy` and then provide its URL as the first argument.

set -xeuo pipefail

DEV_NAMESPACE=default
PROXY=$1
shift

oc get -n openshift-cluster-version deploy
if oc get -n openshift-cluster-version deploy/cluster-version-operator 2>/dev/null; then
    echo "Killing cluster-version-operator"
    oc delete -n openshift-cluster-version deploy/cluster-version-operator
fi

REGISTRY_SVC=$(echo ${PROXY} | sed -e 's,^http://,,')/api/v1/namespaces/openshift-image-registry/services/image-registry/proxy

WHAT=machine-config-daemon
IMGNAME=${USER}/${WHAT}
LOCAL_IMGNAME=localhost/${IMGNAME}
REMOTE_IMGNAME=${DEV_NAMESPACE}/${IMGNAME}
#podman build -t "${LOCALNAME}" -f Dockerfile.${WHAT}
podman push --tls-verify=false "${LOCAL_IMGNAME}" ${REGISTRY_SVC}/${IMGNAME}

oc patch -n openshift-machine-config-operator ds/machine-config-operator -p /dev/stdin <<EOF
spec:
  template:
    spec:
      containers:
        - name: machine-config-daemon
          image: ${REMOTE_IMGNAME}
EOF

@jlebon
Copy link
Member Author

jlebon commented Nov 27, 2018

OK so oc proxy seems like it'd help

Oh neat, didn't know that existed!

Hmm, I was going to suggest just exposing the registry instead but that's not straightforward either.

but podman push expects registries to not have URL components

Unlikely, but could skopeo work around this?

@cgwalters
Copy link
Member

OK, I really find the existing hacking flow to be far too painful to deal with. This patch is about a faster flow for the MCD, which would help me a lot, although for what I'm hacking on I needed to change the MCC too.

So...how about adding a new script which is:

REPO=quay.io/cgwalters ./hack/build-push-run daemon controller

Which does a build, image push, and also edits the configmap json to deploy it?

@openshift-bot
Copy link
Contributor

@jlebon: PR needs rebase.

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.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2018
@cgwalters
Copy link
Member

I spent a little bit more time playing with this after seeing this comment about libvirt+console.

It was easy to extend that to expose the registry, and pushing to it worked! I pushed to the default namspace using the builder SA, but then stumbled over allowing the openshift-machine-config-operator namespace to pull from it. Tried some of the commands from this PR but no luck. Decided to instead push to our namespace and that worked.

Another stumbling block is the long lag times I see after updating the images configmap; it'd be really nice if it was basically instant. Restarting the operator seems to take 2 minutes for leader election.

@ashcrow
Copy link
Member

ashcrow commented Dec 11, 2018

Rebase required.

@jlebon
Copy link
Member Author

jlebon commented Dec 11, 2018

It was easy to extend that to expose the registry, and pushing to it worked!

That's pretty neat! Would you mind opening a separate PR describing this and we can close this one? Even with the lag time, this is still less annoying than the current workflow and less hacky than mine. Alternatively, I guess we can get this patch in and you can work in this new path later?

@openshift-ci-robot openshift-ci-robot added 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. and removed lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 11, 2018
@jlebon
Copy link
Member Author

jlebon commented Dec 11, 2018

OK, rebased and added a few more details! ⬆️

@cgwalters
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 11, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon

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

@ashcrow
Copy link
Member

ashcrow commented Dec 11, 2018

/test e2e-aws

3 similar comments
@ashcrow
Copy link
Member

ashcrow commented Dec 11, 2018

/test e2e-aws

@ashcrow
Copy link
Member

ashcrow commented Dec 11, 2018

/test e2e-aws

@jlebon
Copy link
Member Author

jlebon commented Dec 11, 2018

/test e2e-aws

@ashcrow
Copy link
Member

ashcrow commented Dec 11, 2018

/test e2e-aws

@openshift-merge-robot openshift-merge-robot merged commit bf43413 into openshift:master Dec 11, 2018
@jlebon jlebon deleted the pr/hack branch May 1, 2023 15:32
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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants