Skip to content

Fix arm platform matching#2414

Merged
dmcgowan merged 1 commit intocontainerd:masterfrom
dmcgowan:platform-arm-normalization
Jun 22, 2018
Merged

Fix arm platform matching#2414
dmcgowan merged 1 commit intocontainerd:masterfrom
dmcgowan:platform-arm-normalization

Conversation

@dmcgowan
Copy link
Member

The normalization was being inconsistently applied causing a failure to match some platforms in manifest lists. Fix the matcher and normalization to be more consistent and add changes to parser to prevent the defaulted variants from being set in the platform structure.

Fixes #2409

@codecov-io
Copy link

codecov-io commented Jun 21, 2018

Codecov Report

Merging #2414 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2414      +/-   ##
==========================================
+ Coverage   44.99%   45.06%   +0.06%     
==========================================
  Files          92       92              
  Lines        9398     9413      +15     
==========================================
+ Hits         4229     4242      +13     
- Misses       4486     4488       +2     
  Partials      683      683
Flag Coverage Δ
#linux 49.3% <100%> (+0.06%) ⬆️
#windows 41.34% <100%> (+0.08%) ⬆️
Impacted Files Coverage Δ
platforms/database.go 83.87% <100%> (+3.1%) ⬆️
platforms/platforms.go 83.52% <100%> (-1.48%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ff2748...37ab93e. Read the comment docs.

@dmcgowan
Copy link
Member Author

@stevvooe would this be safe to cherry-pick to 1.1.x?

@stevvooe
Copy link
Member

I didn't think a variant was required for arm64. Perhaps, the bug is in https://github.com/containerd/containerd/blob/master/platforms/cpuinfo.go#L84?

The normalization was being inconsistently applied causing a
failure to match some platforms in manifest lists.
Fix the matcher and normalization to be more consistent and
add changes to parser to prevent the defaulted variants from being
set in the platform structure.

Signed-off-by: Derek McGowan <derek@mcgstyle.net>
@dmcgowan dmcgowan force-pushed the platform-arm-normalization branch from b0ca2dd to 37ab93e Compare June 22, 2018 00:08
@dmcgowan
Copy link
Member Author

dmcgowan commented Jun 22, 2018

I didn't think a variant was required for arm64

It is not required. The normalized form will always be specific now and I updated the Parse to not just directly use the normalized form if the value can already be implied and it wasn't given.

I updated to handle the 8 case like in that link and ensured we aren't overwriting a variant we just don't know about yet (someday arm64/v9?)

@dmcgowan
Copy link
Member Author

ping @tianon can you take a look as well?

Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM; still would be good to have @tianon take a quick look

@tianon
Copy link
Member

tianon commented Jun 22, 2018

Looks sane to me 👍

@dmcgowan dmcgowan merged commit 47a128d into containerd:master Jun 22, 2018
@stevvooe
Copy link
Member

This needs to get reverted. The bug is at https://github.com/containerd/containerd/blob/master/platforms/cpuinfo.go#L84. arm64 should not have a variant.

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.

6 participants