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

Increase compatibility for building of Windows images #1081

Merged
merged 1 commit into from
Mar 20, 2020
Merged

Increase compatibility for building of Windows images #1081

merged 1 commit into from
Mar 20, 2020

Conversation

johnSchnake
Copy link
Contributor

@johnSchnake johnSchnake commented Feb 6, 2020

What this PR does / why we need it:
Currently in a beta state, we are experimenting with building
Windows images so that Sonobuoy can be run on Windows nodes.

This change stops short of adding the Windows build to CI
while we figure out some of the last technical problems there.

Changes include:

  • Utilizing circleci workspaces to load/store docker images across stages
  • Adds a Windows sample plugin
  • Updates the makefile to support building windows images and cleans up some of the makefile structure to build the binaries as appropriate
  • Updates the makefile to build/push images for windows only if PUSH_WINDOWS=true
  • Updates the way the manifest-tool is invoked to use a template instead of the command line options (which provides more flexibility)
  • Adds a flag aggregator-node-selector to gen/run commands in order to help direct where to place the aggregator in mixed-node clusters
  • Modify which command is used to grab the tarball of results from the aggregator depending on whether or not it is a Windows system or not (therefore if bash or powershell should be used)

Which issue(s) this PR fixes

Special notes for your reviewer:
The hardest parts of this are:

  • gettings a Windows cluster
  • getting a Windows docker context

Once those are handled the rest isn't so bad.

Release note:

Several changes related to supporting running on a Windows machine including building Windows compatible images.

@@ -1,4 +1,5 @@
version: 2
version: 2.1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of these changes (like this one) are slightly preemptive. I started from a version where I was making Windows builds (unsuccessfully) and just backtracked on the explicitly windows+CI portions.

Version 2.1 is required here if we want to use the windows orb on circleCI which enables use of the windows executor.

go.sum Outdated Show resolved Hide resolved
manifest_spec.yaml.tmpl Outdated Show resolved Hide resolved
@@ -10,7 +10,7 @@ fi

function image_push() {
echo ${DOCKERHUB_TOKEN} | docker login --username sonobuoybot --password-stdin
make containers push
make push
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eventually (when windows+linux nodes supported) we can't build both on the same machine. So presumably we've make container and make win-container in different machines and loaded the images for this step.

DockerfileWindows Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
result-format: raw
skip-cleanup: true
spec:
image: schnake/windows-plugin:v1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will need to push a sonobuoy version of this

Copy link
Contributor Author

@johnSchnake johnSchnake Mar 20, 2020

Choose a reason for hiding this comment

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

Updated the name; still need to push the image though. I'll do that when this merges.

@@ -141,8 +189,9 @@ func UntarAll(reader io.Reader, destFile, prefix string) (filenames []string, re
}
entrySeq++
mode := header.FileInfo().Mode()
outFileName := path.Join(destFile, header.Name[len(prefix):])
outFileName := filepath.Join(destFile, header.Name)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Double check prefix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this kind of doesnt really matter. The reason being that we take the base name right below this so it is a bit redundant to also strip the prefix.

I'll undo this change for now, but not sure how this may change as both OSs are supported.

@johnSchnake
Copy link
Contributor Author

Need to manually check this again on windows and probably update the manual steps in the release.md; however I think otherwise the code is good. Had manually run it all but that was a few weeks ago. Will post back results when done.

@johnSchnake johnSchnake marked this pull request as ready for review March 19, 2020 19:27
Copy link
Contributor

@zubron zubron left a comment

Choose a reason for hiding this comment

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

Thanks for all your efforts on this, @johnSchnake! I know it's been really awkward to get working. The change so far look good to me, although I haven't tried them out. My understanding is that if this is merged as is, it won't affect any of the existing behaviour, and won't affect the CI scripts until we add in those changes from the other PR? If that's the case, and you're happy with it, I think we can merge this and iterate on it afterwards.

examples/plugins/windows-plugin/README.md Outdated Show resolved Hide resolved
},
goldenFile: filepath.Join("testdata", "multiple-node-selector.golden"),
}, {
name: "Node selector with empty string is OK",
Copy link
Contributor

Choose a reason for hiding this comment

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

Having an empty value for the node selector will produce invalid YAML, do we want to validate that case or rely on the error when we try to use the resulting manifest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the Set method of the flag should handle that; I added a node_selector_test.go that has a unit test for this. As a result, I removed this test case entirely since it (1) shouldn't occur and (2) produces invalid yaml anyways.

@johnSchnake johnSchnake changed the title WIP Increase compatibility for building of Windows images Increase compatibility for building of Windows images Mar 20, 2020
Windows images so that Sonobuoy can be run on Windows nodes.

This change stops short of adding the Windows build to CI
while we figure out some of the last technical problems there.

Changes include:
 - Utilizing circleci workspaces to load/store docker images across stages
 - Adds a Windows sample plugin
 - Updates the makefile to support building windows images and cleans up
some of the makefile structure to build the binaries as appropriate
 - Updates the makefile to build/push images for windows only if PUSH_WINDOWS=true
 - Updates the way the manifest-tool is invoked to use a template instead
of the command line options (which provides more flexibility)
 - Adds a flag `aggregator-node-selector` to gen/run commands in order to
help direct where to place the aggregator in mixed-node clusters
 - Modify which command is used to grab the tarball of results from the
aggregator depending on whether or not it is a Windows system or not
(therefore if bash or powershell should be used)

Signed-off-by: John Schnake <jschnake@vmware.com>
@johnSchnake
Copy link
Contributor Author

Based on @zubron 's last comment I'm going to merge once CI goes green. I fixed up some problems I found in the makefile, added 2 more tests (and removed one), and made some doc tweaks.

@johnSchnake johnSchnake merged commit 1965569 into vmware-tanzu:master Mar 20, 2020
@johnSchnake johnSchnake deleted the windowsCompat branch March 20, 2020 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants