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

POC/WIP Windows image #1071

Closed
wants to merge 1 commit into from
Closed

POC/WIP Windows image #1071

wants to merge 1 commit into from

Conversation

johnSchnake
Copy link
Contributor

What this PR does / why we need it:
First draft + tinkering of building Sonobuoy to run on Windows nodes.

Which issue(s) this PR fixes

Special notes for your reviewer:
I'll be just using this as a reference as I put up smaller PRs that fix misc issues I ran into.

@@ -65,6 +64,13 @@ func (c *SonobuoyClient) RetrieveResults(cfg *RetrieveConfig) (io.Reader, <-chan
return nil, nil, errors.Wrap(err, "failed to get the name of the aggregator pod to fetch results from")
}

// DEBUG; dont assume Linux os
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to still have some logic to choose the right tar command.

I think I just need to GET the aggregator pod, see which node it is on, GET the node and check its labels for the OS.

Better idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

The only other thing I can think of right now is to have a utility script or something with the same name that exists inside both the Linux and Windows images that would take care of forming the correct tar command - similarly to how we're now relying on the Dockerfile to know how to run the aggregator. I don't think that would work though due to the differences in client/server versions. It seems much more likely for the retrieve command that you could be using a newer client version against an older server version.

I think checking the node labels is probably the way to go.

return
//returnErr = fmt.Errorf("non-zero data %v read after tar EOF", b[i])
//return
fmt.Fprintf(os.Stderr, "non-zero data %v read after tar EOF", b[i])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to revert this change. Originally I thought there was a tar difference on Windows so I wanted to see this data that was left around. In the end it was just an error on my part.

@@ -135,14 +142,16 @@ func UntarAll(reader io.Reader, destFile, prefix string) (filenames []string, re
header, err := tarReader.Next()
if err != nil {
if err != io.EOF {
//fmt.Println("schnake next err")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revert.

@johnSchnake
Copy link
Contributor Author

@zubron Still a few fixups but ready for early review if you want to take a look.

architecture: amd64
os: linux
-
image: REGISTRY/sonobuoy-amd64:TAG
Copy link
Contributor Author

Choose a reason for hiding this comment

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

s/amd64/arm64

@@ -0,0 +1,28 @@
podSpec:
nodeSelector:
Copy link
Contributor

Choose a reason for hiding this comment

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

Will you be including a README or anything along with this to explain what is required to create a Windows plugin or will that be included as separate docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I'll add a readme to this plugin folder and talk about windows specific points of interest. Seems like a reasonable place to put it; may add a link elsewhere in docs to point to this as well.

@@ -87,7 +89,7 @@ func GatherResults(waitfile string, url string, client *http.Client, stopc <-cha
select {
case <-ticker:
if resultFile, err := ioutil.ReadFile(waitfile); err == nil {
logrus.WithField("resultFile", string(resultFile)).Info("Detected done file, transmitting result file")
resultFile = bytes.TrimSpace(resultFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is removing the logging line intentional?

FROM BASEIMAGE
MAINTAINER John Schnake "[email protected]"

ADD build/windows/amd64/sonobuoy.exe /sonobuoy.exe
Copy link
Contributor

Choose a reason for hiding this comment

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

Should build/windows/amd64/sonobuoy.exe be BINARY as that's what the Makefile is expecting to replace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 thanks; started out with hardcoding and didnt revert that part on the next iteration I guess.

@@ -65,6 +64,13 @@ func (c *SonobuoyClient) RetrieveResults(cfg *RetrieveConfig) (io.Reader, <-chan
return nil, nil, errors.Wrap(err, "failed to get the name of the aggregator pod to fetch results from")
}

// DEBUG; dont assume Linux os
Copy link
Contributor

Choose a reason for hiding this comment

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

The only other thing I can think of right now is to have a utility script or something with the same name that exists inside both the Linux and Windows images that would take care of forming the correct tar command - similarly to how we're now relying on the Dockerfile to know how to run the aggregator. I don't think that would work though due to the differences in client/server versions. It seems much more likely for the retrieve command that you could be using a newer client version against an older server version.

I think checking the node labels is probably the way to go.

@@ -10,7 +10,7 @@ fi

function image_push() {
echo ${DOCKERHUB_TOKEN} | docker login --username sonobuoybot --password-stdin
make containers 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.

In CI the images will be created in another step (linux+windows) and the publishing step will just need to push.

@johnSchnake
Copy link
Contributor Author

Since I am blocked currently on the actual circleCI work but not the windows stuff itself I'm going to open a separate PR and split those up.

This would mean one PR makes Windows nodes supported through a manual build process (which I'll document).

The other PR will tie it into CircleCI.

@johnSchnake
Copy link
Contributor Author

I believe this is outdated and #1081 takes over adding the windows stuff. Some of the other code was moved into another PR that already merged. If mistaken we can just reopen this.

@johnSchnake johnSchnake reopened this Mar 18, 2020
@johnSchnake
Copy link
Contributor Author

So I think the other PR has most of this code but this one also adds stuff to the circleci config.

Intent is to first get the other PR fixed up and merged, then consider this one in some capacity. The other will enable the manual building of the windows image which may be enough for now as a POC effectively. Then this can be revisited if the windows stuff gets utilized

Currently in a beta state, we are experimenting with building
Windows images so that Sonobuoy can be run on Windows nodes.

Signed-off-by: John Schnake <[email protected]>
@stale
Copy link

stale bot commented Nov 4, 2020

There has not been much activity here. We'll be closing this issue if there are no follow-ups within 15 days.

@stale stale bot added the misc/wontfix label Nov 4, 2020
@stale stale bot closed this Nov 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Windows nodes
2 participants