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.

Sign up for GitHub

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

Jump to bottom

Add os_version and os_features to Image #21272

Merged
merged 1 commit into from
Apr 5, 2016
Merged

Add os_version and os_features to Image #21272

merged 1 commit into from
Apr 5, 2016

Conversation

jstarks
Copy link
Contributor

@jstarks jstarks commented Mar 17, 2016

@aaronlehmann @dmp42

These fields are needed to specify the exact version of Windows that an
image can run on. They may be useful for other platforms in the future.

This also changes image.store.Create to validate that the loaded image is
supported on the current machine. This change affects Linux as well, since
it now validates the architecture and OS fields.

@jstarks
Copy link
Contributor Author

jstarks commented Mar 17, 2016

Note that I have not even tested this yet, but in interest of time I wanted to get the basic idea out there before I leave for the day.

@jstarks jstarks force-pushed the jstarks/manifest_updates branch from 4f98226 to 76390ae Compare March 17, 2016 06:23
@jstarks
Copy link
Contributor Author

jstarks commented Mar 17, 2016

The new check to verify the architecture shows that there is an interesting conflict: there is clearly some expectation that x86_64 is the way to write the 64-bit x86-based architecture (that's what is written by docker version, etc.), but the image construction logic all seems to use runtime.GOARCH for Image.Architecture, which writes amd64. I had to fix emptyfs to use amd64 for emptyfs to properly import.

If there are any images out there that somehow got architecture = x86_64, they will be broken by this change. If we're worried about this, we could special case x86_64 and treat it the same as amd64.

@jstarks jstarks force-pushed the jstarks/manifest_updates branch from 76390ae to af5802f Compare March 17, 2016 15:01
@jstarks
Copy link
Contributor Author

jstarks commented Mar 17, 2016

Still failures with the change to emptyfs. I decided to add x86_64 as an acceptable alias for amd64.

@jstarks
Copy link
Contributor Author

jstarks commented Mar 17, 2016

@jhowardmsft Not sure what's going on with windowsTP4...

@lowenna
Copy link
Member

lowenna commented Mar 17, 2016

Restarted TP4 after rebooting a few of the machines

@jstarks
Copy link
Contributor Author

jstarks commented Mar 17, 2016

I'm also confused about janky; it seems like it's done in the details page...

@lowenna
Copy link
Member

lowenna commented Mar 17, 2016

It is. Looks like Jenkins didn't get the completion notification.

@thaJeztah
Copy link
Member

@jstarks looks like this needs a rebase now 😢

@jstarks jstarks force-pushed the jstarks/manifest_updates branch 2 times, most recently from 54618b1 to bcf6c4d Compare March 29, 2016 00:04
@icecrime icecrime added this to the 1.11.0 milestone Mar 30, 2016
@tiborvass
Copy link
Contributor

I wonder if we should have an OS or Platform struct field with more detail in it instead of multiple os-specific fields.

Anyway, otherwise, this makes sense to me.

aaronlehmann
aaronlehmann reviewed Mar 30, 2016
View reviewed changes
image/image.go
RootFS *RootFS `json:"rootfs,omitempty"`
History []History `json:"history,omitempty"`
OSVersion string `json:"os_version,omitempty"` // os_version similar to docker_version; BIKESHED: osversion, os.version
OSFeatures []string `json:"os_features,omitempty"` // BIKESHED: os_features, os.features
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we went with os.version and os.features in distribution/distribution#1534, can we use the same naming convention here?

@icecrime
Copy link
Contributor

This PR might not make it in the 1.11 Linux release, which from my understanding is acceptable. Furthermore, Windows builds of the daemon are built from master until Windows Server 2016 reaches GA, so it should be fine to merge outside of the release process.

@jstarks If you feel strongly that this is desirable to have in 1.11, would you be available to update this PR to address the comments above? As a last resort, a maintainer (@aaronlehmann? 😇) could perhaps carry?

Let me know, thanks!

@icecrime
Copy link
Contributor

Just confirmed that this can be merged out of band from the release process: removing from milestone.

@icecrime icecrime removed this from the 1.11.0 milestone Mar 31, 2016
@jstarks
Add os_version and os_features to Image
194eaa5 
These fields are needed to specify the exact version of Windows that an
image can run on. They may be useful for other platforms in the future.

This also changes image.store.Create to validate that the loaded image is
supported on the current machine. This change affects Linux as well, since
it now validates the architecture and OS fields.

Signed-off-by: John Starks <jostarks@microsoft.com>
@jstarks jstarks force-pushed the jstarks/manifest_updates branch from bcf6c4d to 194eaa5 Compare April 4, 2016 20:15
@thaJeztah
Copy link
Member

ping @aaronlehmann I see the PR was updated

@jstarks please note that maintainers/reviewers will not be automatically notified if changes are pushed, only when new comments are added, so if you push changes that need a review, it's (unfortunately) needed to "ping" people manually

@jstarks
Copy link
Contributor Author

jstarks commented Apr 4, 2016

@thaJeztah Ah, I did not know that, thanks. I'm still a GitHub newb, I guess.

Yes, the latest push should resolve all of @aaronlehmann's feedback.

@aaronlehmann
Copy link
Contributor

LGTM

@jstarks
Copy link
Contributor Author

jstarks commented Apr 5, 2016

@jhowardmsft

@lowenna
Copy link
Member

lowenna commented Apr 5, 2016

LGTM

@lowenna lowenna merged commit fc9912f into moby:master Apr 5, 2016
@jstarks jstarks deleted the jstarks/manifest_updates branch April 6, 2016 01:39
@clnperez
Copy link
Contributor

clnperez commented Apr 6, 2016

😢

---> Making bundle: .ensure-emptyfs (in bundles/1.11.0-dev/test-integration-cli)
++++ tar -cC bundles/1.11.0-dev/test-integration-cli/emptyfs .
++++ docker load
Error response from daemon: image is for architecture x86_64, expected ppc64le
/go/src/github.com/docker/docker/hack/make.sh: line 308: local: can only be used in a function

@thaJeztah
Copy link
Member

@clnperez so this broke the ppc64le builds? Can you open an issue for tracking?

@clnperez
Copy link
Contributor

clnperez commented Apr 6, 2016

@thaJeztah Yup. Sure thing.

@tophj-ibm tophj-ibm mentioned this pull request Apr 6, 2016
Merged
@clnperez
Copy link
Contributor

clnperez commented Apr 7, 2016

@thaJeztah, @tophj-ibm has already submitted a PR to take care of it!

@thaJeztah
Copy link
Member

I noticed :-)

tophj-ibm added a commit to tophj-ibm/moby that referenced this pull request Apr 8, 2016
@tophj-ibm
Fix .ensure-emptyfs on non-x86_64 architectures
1f59bc8 
Now that we are checking if the image and host have the same architectures
via moby#21272, this value should be null so that the test passes on non-x86
machines

Signed-off-by: Christopher Jones <tophj@linux.vnet.ibm.com>
@thaJeztah thaJeztah mentioned this pull request Jan 26, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
area/distribution platform/windows status/2-code-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@jstarks @lowenna @thaJeztah @tiborvass @icecrime @aaronlehmann @clnperez @calavera @GordonTheTurtle