Enable kernel2 builds in pipeline #5753

smacfarlane · 2018-10-16T22:09:39Z

This PR enables x86_64-linux-kernel2 builds in the release pipeline. This will result in a new set of Habitat artifacts that need to be validated on a CentOS6-era Linux system.

Blocked by #5752

TODO:

Enable kernel2 rootless studio builds
Update install.sh script to take --target
Document x86_64-linux-kernel2 validation steps

thesentinels · 2018-10-16T22:09:43Z

Thanks for the pull request! Here is what will happen next:

Your PR will be reviewed by the maintainers
If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

christophermaier

@smacfarlane Looking good so far 👍

christophermaier · 2018-10-18T13:36:08Z

.buildkite/release_pipeline.yaml

+
+  - label: ":linux: :see_no_evil: :habicat: core/hab/kernel2"
+    command: .buildkite/scripts/build_component.sh hab
+    concurrency_group: "habitat/release/x86_64-linux-kernel2"


What's the intention behind the concurrency group for just the kernel2 builds? And also without a corresponding concurrecy key?

That was in part a copy/paste from the bootstrap pipeline where we use it to enforce isolation. I chatted with @scotthain about this after I initially published this branch and we decided we need to think on it a bit when we didn't have end-of-day pudding brain 😹 IIRC the question came down to: Do we need to ensure isolation between our two Linux builds for the release?

I don't think we do... the only kind of isolation I think we need is the sequencing, but that's taken care of using the various wait steps.

The only thing we're currently using concurrency on is the macOS builder, and then only because the builder is a single, long-lived machine, and we've had issues with our Travis-based release of having concurrent builds stepping on each other. It doesn't feel like there's a similar concern with the kernel 2 stuff, but I may be missing something; you guys have been more in that work than I have.

☕️ Kicking in now. My concern was that if a kernel-current and kernel2 build were scheduled on the same host, you would end up with failed or corrupted builds because hab pkg build is going to attempt to destroy any existing studio at the current path, which would be the same for the two Targets.

We could probably work around this by naming the studio (-r) with the Target as part of the studio name, but it might just be safer to enforce them executing on separate hosts.

I'm not sure if it's possible for builds to be scheduled on the same host, I'm not familiar enough with how it's set up, so this may be a non-concern.

I think the path should be different, though, since they'd be different Buildkite jobs, and I think that's part of the path.

As for being scheduled on the same host, I think that depends on how the agent is configured, which is a question for @scotthain, methinks.

After more thoughts, we shouldn't need the concurrency group in this case. (And if there's something we've manage to forget/nuke from our brains, we can add it back)

Also as a side note, if we don't cut the concurrency down to 1 (as we do in some other pipelines as a forced limiting factor) there's no real point in this case :)

.buildkite/release_pipeline.yaml

christophermaier · 2018-10-18T13:39:12Z

.buildkite/release_pipeline.yaml

@@ -19,6 +19,14 @@ steps:
    command: .buildkite/scripts/build_component.sh hab
    agents:
      queue: habitat-release
+
+  - label: ":linux: :see_no_evil: :habicat: core/hab/kernel2"


I'd separate the "kernel2" text from the package name, just so it's very clear that "kernel2" isn't some kind of weird version string 😄

Also, is kernel 2 "evil"? 🤣

Good point, I'll fix that.

A little bit 😂 That just happened to be in front of me when I stopped looking for a good representative icon for the kernel2 builds. It was a small delighter for myself at the end of the day 😄 It should change when we decide on an icon to use.

I don't necessarily object 😄 I'm a big fan of using "delighter" emoji in Buildkite.

.buildkite/release_pipeline.yaml

christophermaier · 2018-10-18T13:46:23Z

.buildkite/scripts/build_component.sh

    fi
+    declare -g hab_binary


Ha, I guess it makes more sense to declare this outside the if/else 😄

christophermaier · 2018-10-18T13:47:29Z

.buildkite/scripts/build_component.sh

    echo "--- :habicat: Using $(${hab_binary} --version)"
 }

+install_hab_kernel2_binary() {


The comments above about this being a temporary step may be better attached to this function directly.

christophermaier · 2018-10-18T13:48:42Z

.buildkite/scripts/build_mac_release.sh

-# TODO (CM): Replace "macOS" below with ${pkg_target:?} once that's in
-# hab-plan-build (see https://github.com/habitat-sh/habitat/pull/5373)
-echo "<br>* ${pkg_ident:?} (macOS)" | buildkite-agent annotate --append --context "release-manifest"
+echo "<br>* ${pkg_ident:?} (${pkg_target:?})" | buildkite-agent annotate --append --context "release-manifest"


christophermaier · 2018-10-18T13:48:50Z

.buildkite/scripts/build_component.sh

-# TODO (CM): Replace "Linux" below with ${pkg_target:?} once that's in
-# hab-plan-build (see https://github.com/habitat-sh/habitat/pull/5373)
-echo "<br>* ${pkg_ident} (Linux)" | buildkite-agent annotate --append --context "release-manifest"
+echo "<br>* ${pkg_ident:?} (${pkg_target:?})" | buildkite-agent annotate --append --context "release-manifest"


.buildkite/scripts/build_component.sh

scotthain · 2018-10-19T16:18:25Z

.buildkite/release_pipeline.yaml

+
+  - label: ":linux: :see_no_evil: :habicat: core/hab/kernel2"
+    command: .buildkite/scripts/build_component.sh hab
+    concurrency_group: "habitat/release/x86_64-linux-kernel2"


Also as a side note, if we don't cut the concurrency down to 1 (as we do in some other pipelines as a forced limiting factor) there's no real point in this case :)

christophermaier · 2018-10-23T16:49:53Z

@smacfarlane This continues to look really nice 👍

How close do you think we are to having this not be "WIP"?

smacfarlane · 2018-10-24T15:14:47Z

@christophermaier Very soon. The build portion of the pipeline looks good. I keep hitting what look like infrastructure issues so haven't had a successful end-to-end run without failures yet. I'd like to do a bit more digging to confirm that the errors are unrelated to the changes in the scripts before removing the WIP.

smacfarlane · 2018-10-31T16:01:44Z

@christophermaier I've removed the WIP 🎉 🌮 I'm working on the docker changes noted in the initial PR. While not a strict blocker to this one, I would like to make sure both changes make it in before our next release so they should be merged around the same time.

smacfarlane · 2018-11-01T16:20:41Z

@christophermaier One thing to note, I think we'll have to do a manual release of the x86_64-linux-kernel2 docker studio, with a follow on PR to enable that in the pipeline post-release. This is because the image building process relies on a published hab release in bintray, which doesn't occur until after the docker images are built and uploaded.

christophermaier · 2018-11-05T16:48:16Z

.buildkite/release_pipeline.yaml

+    prompt: |
+      x86_64-linux-kernel2 launcher will need to be built by hand.  If 
+      you are promoting launchers in the following step, please build 
+      and upload and x86_64-linux-kernel2 launcher before continuing.


Is this because of a lack of kernel2 workers in Builder (which would normally be building this)?

(Adding or linking to explicit directions for building a kernel2 launcher would be good here.)

Also, why 🍡? 😄

Correct. I will add those steps to the Release documentation and link to them.

I was thinking about how good a dango would be at the time I was working on this. I am happy to report that that particular issue was resolved 😄 Also, three little launchers all lined up?(I just made that up, but I kinda like it)

christophermaier · 2018-11-05T16:51:17Z

.buildkite/scripts/bintray_publish.sh

+# Linux x86-64-linux-kernel2 Publish
+release=$(buildkite-agent meta-data get "hab-release-x86_64-linux-kernel2")
+echo "--- :linux: :two: Publishing x86-64-linux-kernel2 'hab' ${version}-${release} on Bintray"
+publish "https://api.bintray.com/content/habitat/${bintray_repository}/hab-x86_64-linux-kernel2/${version}-${release}/publish"


I think we're at the point where we want to refactor the publish function to take repository, target, version, and release arguments, and generate the complete URL internally. Otherwise, we've duplicated the URL creation logic four times.

👍 Will refactor

christophermaier · 2018-11-05T16:54:06Z

.buildkite/scripts/bintray_upload.sh


-hab_artifact=$(buildkite-agent meta-data get "hab-artifact")
+# TODO (SM): Should we specify Target for all of our platforms? 


Probably so, particularly once we get the Windows builds working in Buildkite. Might as well take care of that now.

christophermaier · 2018-11-05T16:55:39Z

.buildkite/scripts/bintray_upload_macos.sh

@@ -24,7 +24,7 @@ sudo hab pkg install \
     core/hab-bintray-publish

 echo "--- :buildkite: Retrieving macOS core/hab artifact"
-hab_artifact=$(buildkite-agent meta-data get "hab-artifact-macos")
+hab_artifact=$(buildkite-agent meta-data get "hab-artifact-x86_64-darwin")


Wonder if we can get a bit more consistency, and maybe be able to abstract some of these metadata calls into functions, if we require a package target environment variable to be set everywhere, and then take advantage of that here, rather than hardcoding?

I started to look at this because I think it's the right idea, but I've hit a bit of a snag. We use the meta-data get for both the current $pkg_target, but in the case of bintray_publish for all $pkg_targets.

Creating a function that always requires the target be passed doesn't get us much (anything?) from requiring a Target environment variable. Having the target be optional I'd argue is worse because it would be pretty easy to miss a spot where it's necessary and hard to debug in the pipeline.

We could split out bintray_publish to act only on a single target, and have a step for each target... I think this is what I'll try. Refactoring all scripts to act on a single target where appropriate. It means we'd need N workers to run each curl rather than 1 worker to run N curls. I think the failure case of one not working isn't any worse than current state either.

I may have been overly prescriptive... the function doesn't need to operate on the specific environment variable, but we could take advantage of the environment variable and pass it in as an argument.

One thing this would allow us to do is hide the nature of the keys behind a function. For example, we do that here:

habitat/.buildkite/scripts/shared.sh

Lines 47 to 61 in 58f19cf

# Abstracts the logic (such as it is) for whether we're doing a "fake"

# release or not.

set_fake_release() {

local release=${1}

buildkite-agent meta-data set fake-release "${release}"

}

is_fake_release() {

buildkite-agent meta-data exists fake-release

}

get_fake_release() {

buildkite-agent meta-data get fake-release

}

Every place that needs to operate on the metadata for a "fake release" doesn't need to know what the metadata key is (or that it's even stored in Buildkite metadata). It feels like we're getting to the point where we're making more and more use of the metadata feature, to the point where it could become confusing and error prone to be replicating all this logic across the various scripts.

Then here, we'd just have a list of targets we care about (not even environment variables, necessarily) and we'd just iterate through them, operating on each appropriate package in turn.

christophermaier · 2018-11-05T16:56:26Z

.buildkite/scripts/build_component.sh

+# doesn't emit pkg_target. Until we've sufficiently bootstrapped ourselves
+# we need to set it. This can be removed when studio-ci-common pulls 0.63.0 
+# or newer. This is safe to do because the x86_64-linux-kernel2 builds will
+# already have this value set.


Might be good to create an issue for this and add a link here.

baumanj

Some relatively minor issues that you can do what you will with.

Additionally, I notice :? used in a number of places (e.g. ${hab_binary:?}). In contexts where set -u is present, this is redundant, but not harmful. However, it also isn't used consistently, so I think it would be preferable to choose to either alway or never use that form.

Ditto to the other 2 instances

baumanj · 2018-11-06T16:13:22Z

.buildkite/scripts/shared.sh

+# new studio.
+set_hab_binary() {
+    echo "--- :thinking_face: Determining which 'hab' binary to use"
+    # This ensure the hab cli we use going forward has the correct


ensure → ensures

baumanj · 2018-11-06T16:18:05Z

.buildkite/scripts/shared.sh

+            ;;
+        x86_64-linux-kernel2)
+            install_hab_kernel2_binary
+            hab_binary="$(which hab-x86_64-linux-kernel2)"


which is almost certainly fine here, but just FYI it's not POSIX, so command -v is a bit safer in general and has the benefit of being a bash builtin.

baumanj · 2018-11-06T16:19:33Z

.buildkite/scripts/shared.sh

+        # and thus do not require a `--channel` option. However, they
+        # should be coming from the release channel, and should be the
+        # same packages built previously in this same release pipeline.
+        hab_ident=$(get_hab_ident "${pkg_target}")


Is this intended to be a global, or would it be better to declare local?

baumanj · 2018-11-06T16:27:41Z

.buildkite/scripts/shared.sh

+    else
+        echo "Buildkite metadata NOT found; using previously-installed hab binary: $hab_binary"
+    fi
+    declare -g hab_binary


These declare -gs are both unnecessary (without local, the variables will be global), but being explicit isn't a bad thing.

However, it is a bit confusing to have the declare down here. Why not up next to local pkg_target?

baumanj · 2018-11-06T16:35:35Z

.buildkite/scripts/shared.sh

+        sudo "${hab_binary:?}" pkg install "${hab_ident}"
+        sudo "${hab_binary:?}" pkg install "$(get_studio_ident $pkg_target)"
+        hab_binary="/hab/pkgs/${hab_ident}/bin/hab"
+        declare -g new_studio=1


I tend to prefer

Suggested change

declare -g new_studio=1

declare -g new_studio=true

since it allows consumers to write

if "$new_studio"; then

rather than

if [ "$new_studio" -eq 1]; then

or

if [ -n "$new_studio" ]; then

or some variation depending on what we consider truth for that variable.

baumanj · 2018-11-06T16:45:46Z

.buildkite/scripts/shared.sh

+    curl "$hab_src_url" -o hab-x86_64-linux-kernel2
+    sudo mv hab-x86_64-linux-kernel2 /bin/hab-x86_64-linux-kernel2
+    sudo chmod +x /bin/hab-x86_64-linux-kernel2
+    popd 


I like this idiom a little better since it makes it impossible to forget the popd and you don't need the >/dev/null since cd has no output.

( cd "$tempdir" curl "$hab_src_url" -o hab-x86_64-linux-kernel2 sudo mv hab-x86_64-linux-kernel2 /bin/hab-x86_64-linux-kernel2 sudo chmod +x /bin/hab-x86_64-linux-kernel2 )

baumanj · 2018-11-06T16:47:27Z

.buildkite/scripts/shared.sh

+    sudo mv hab-x86_64-linux-kernel2 /bin/hab-x86_64-linux-kernel2
+    sudo chmod +x /bin/hab-x86_64-linux-kernel2
+    popd 
+    rm -rf "$tempdir" >/dev/null


If you need the -f here, I think there's something wrong.

Also, I'd omit the >/dev/null. If this outputs anything, it's because something is amiss.

Signed-off-by: Scott Macfarlane <macfarlane.scott@gmail.com>

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

Signed-off-by: Scott Macfarlane <macfarlane.scott@gmail.com>

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

…_upload Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

Obvious fix; these changes are the result of automation not creative thinking.

smacfarlane added the T-DO-NOT-MERGE label Oct 16, 2018

smacfarlane requested review from christophermaier and scotthain October 16, 2018 22:09

smacfarlane requested a review from fnichol as a code owner October 16, 2018 22:09

christophermaier reviewed Oct 18, 2018

View reviewed changes

scotthain approved these changes Oct 19, 2018

View reviewed changes

smacfarlane mentioned this pull request Oct 19, 2018

Add -t to support installing alternate PackageTarget cli #5770

Merged

smacfarlane force-pushed the sm/enable-kernel2-builds-in-pipeline branch 2 times, most recently from f9f088d to c6fc557 Compare October 23, 2018 16:25

smacfarlane requested review from raskchanky and reset as code owners October 23, 2018 22:50

smacfarlane force-pushed the sm/enable-kernel2-builds-in-pipeline branch from b61fdf6 to f3c191b Compare October 31, 2018 15:13

smacfarlane changed the title **WIP** Enable kernel2 builds in pipeline Enable kernel2 builds in pipeline Oct 31, 2018

smacfarlane mentioned this pull request Nov 1, 2018

Add target to Docker image names for studio #5805

Merged

christophermaier reviewed Nov 5, 2018

View reviewed changes

smacfarlane requested a review from baumanj as a code owner November 6, 2018 00:31

baumanj approved these changes Nov 6, 2018

View reviewed changes

smacfarlane force-pushed the sm/enable-kernel2-builds-in-pipeline branch from 22896e1 to 6ad936d Compare November 14, 2018 19:05

smacfarlane requested a review from elliott-davis as a code owner November 14, 2018 20:57

smacfarlane removed the T-DO-NOT-MERGE label Nov 14, 2018

smacfarlane and others added 5 commits November 16, 2018 08:27

Refactor build component for kernel2 support

ee43397

Signed-off-by: Scott Macfarlane <macfarlane.scott@gmail.com>

Update mac build to log pkg_target for parity with Linux

563b841

Signed-off-by: Scott Macfarlane <macfarlane.scott@gmail.com>

Enable kernel2 builds of some components

05cd2ff

Signed-off-by: Scott Macfarlane <macfarlane.scott@gmail.com>

Add kernel2 support to bintray-publish

0f53edb

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

Enable kernel2 builds for bintray-publish

d07ad97

Signed-off-by: Scott Macfarlane <macfarlane.scott@gmail.com>

smacfarlane and others added 8 commits November 16, 2018 08:27

Add iconography for kernel2

49b86b7

Signed-off-by: Scott Macfarlane <macfarlane.scott@gmail.com>

Remove concurrency group for kernel2

df8fbeb

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

Publish kernel2 bintray artifact

2e579ae

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

Move set_hab_binary to shared.sh, so that it can be reused in bintray…

84582d0

…_upload Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

Set hab binary based on Target for current step

e72e254

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

Plumb pkg_target through metadata

a22d273

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

Set pkg_target for x86_64-linux post-build

8ed736b

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

Add kernel2 to publish_launchers

3ce5fc0

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

smacfarlane force-pushed the sm/enable-kernel2-builds-in-pipeline branch from 9dc1f2c to f64db10 Compare November 16, 2018 16:34

smacfarlane added 10 commits November 16, 2018 10:15

Add additional context to hint string

2aada5b

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

Add kernel2 launcher manual build requirement note

99c8d33

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

Refactor bintray publish

02a754e

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

Always pass target into bintray_upload

5b9d367

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

Refactor metadata access and require BUILD_PKG_TARGET where appropriate

562fc88

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

Create x86_64-linux-kernel2 launcher build instructions

0ee85a3

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

Add target to dockerhub_upload step

16591ea

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

Use bash to run the install script

f5c5a38

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

Fix dockerfile and build issues

ac5ebe2

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

Add information about validating x86_64-linux-kernel2

b5e901d

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

smacfarlane force-pushed the sm/enable-kernel2-builds-in-pipeline branch from ef1a9ce to b5e901d Compare November 16, 2018 18:16

smacfarlane merged commit 62d41af into master Nov 16, 2018

smacfarlane deleted the sm/enable-kernel2-builds-in-pipeline branch November 16, 2018 21:02

chef-ci added a commit that referenced this pull request Nov 16, 2018

Update CHANGELOG.md with details from pull request #5753

ae9f2d2

Obvious fix; these changes are the result of automation not creative thinking.

smacfarlane mentioned this pull request Nov 16, 2018

Update homebrew release to utilize metadata helper functions #5860

Merged


		hab_artifact=$(buildkite-agent meta-data get "hab-artifact")
		# TODO (SM): Should we specify Target for all of our platforms?

	# Abstracts the logic (such as it is) for whether we're doing a "fake"
	# release or not.

	set_fake_release() {
	local release=${1}
	buildkite-agent meta-data set fake-release "${release}"
	}

	is_fake_release() {
	buildkite-agent meta-data exists fake-release
	}

	get_fake_release() {
	buildkite-agent meta-data get fake-release
	}

Enable kernel2 builds in pipeline #5753

Enable kernel2 builds in pipeline #5753

Conversation

smacfarlane commented Oct 16, 2018 • edited

thesentinels commented Oct 16, 2018

christophermaier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christophermaier commented Oct 23, 2018

smacfarlane commented Oct 24, 2018

smacfarlane commented Oct 31, 2018

smacfarlane commented Nov 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baumanj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smacfarlane commented Oct 16, 2018 •

edited