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

set MediaType in BlobStatus from VerifyImageStatus #4511

Merged
merged 1 commit into from
Jan 9, 2025
Merged

set MediaType in BlobStatus from VerifyImageStatus #4511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/pillar/cmd/volumemgr/blob.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,7 @@ func lookupOrCreateBlobStatus(ctx *volumemgrContext, blobSha string) *types.Blob
State: vs.State,
Path: vs.FileLocation,
Size: uint64(vs.Size),
MediaType: vs.MediaType,
Copy link
Member

Choose a reason for hiding this comment

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

I think it's important to note that sometimes we set the MediaType based on the DownloaderStatus as well:

eve/pkg/pillar/cmd/volumemgr/blob.go

Line 194 in 5f70ef8

if blob.MediaType == "" {

This was added in a commit by @deitch: 5f70ef8

It looks like the change proposed in this PR corresponds to the change in the mentioned commit—both aim to ensure that the MediaType is set when it's absent for some reason. As far as I understand, if we use the proposed approach, the previous change becomes unnecessary. Or am I wrong? In any case, we need to understand how these parts of the code that set MediaType correspond to each other.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is the hardest part. This seems to make sense to me, but the interplay between them is hard to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with both of you. But I also don't see any harm in setting it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we have any test to reproduce the bug / verify that the fix is working?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@OhmSpectator which test does this refer to?

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/zededa/bug-reproduction/tree/main/ci-197 here it is, but it requires some extra setup, if I remember correctly

Copy link
Contributor

Choose a reason for hiding this comment

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

@OhmSpectator , I've seen some times (not often) an error of mismatching layers while deploying container applications on arm devices. However, I could never reproduce precisely... what I do know is that when it happens it's because of some application/vnd.lfedge.disk.layer.v1+raw layers (the disk images), that we know that these media types ares not integrated to containerd...

I think these changes are harmless (I did few tests on my side), and as @europaul mentioned, it can help to debug, I will approve to let Eden tests run... let's see how it goes...

CurrentSize: vs.Size,
TotalSize: vs.Size,
Progress: 100,
Expand Down
4 changes: 3 additions & 1 deletion pkg/pillar/types/blob.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func (status BlobStatus) LogCreate(logBase *base.LogObject) {
logObject.CloneAndAddField("state", status.State.String()).
AddField("datastoreid-uuids", uuids).
AddField("size-int64", status.Size).
AddField("blobtype-string", status.MediaType).
AddField("mediatype", status.MediaType).
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why, but it looks like there is an implicit convention of "name-type" for all these fields. Shouldn't we at least follow it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tbh I have no idea why it's there. And some fields like state don't follow it.

AddField("refcount-int64", status.RefCount).
AddField("has-verifier-ref-bool", status.HasVerifierRef).
AddField("has-downloader-ref-bool", status.HasDownloaderRef).
Expand All @@ -116,6 +116,7 @@ func (status BlobStatus) LogModify(logBase *base.LogObject, old interface{}) {
oldStatus.Size != status.Size {

logObject.CloneAndAddField("state", status.State.String()).
AddField("mediatype", status.MediaType).
AddField("refcount-int64", status.RefCount).
AddField("size-int64", status.Size).
AddField("has-verifier-ref-bool", status.HasVerifierRef).
Expand Down Expand Up @@ -145,6 +146,7 @@ func (status BlobStatus) LogDelete(logBase *base.LogObject) {
logObject := base.EnsureLogObject(logBase, base.BlobStatusLogType, status.RelativeURL,
nilUUID, status.LogKey())
logObject.CloneAndAddField("state", status.State.String()).
AddField("mediatype", status.MediaType).
AddField("refcount-int64", status.RefCount).
AddField("size-int64", status.Size).
AddField("has-verifier-ref-bool", status.HasVerifierRef).
Expand Down
6 changes: 6 additions & 0 deletions pkg/pillar/types/verifiertypes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ func (config VerifyImageConfig) LogCreate(logBase *base.LogObject) {
return
}
logObject.CloneAndAddField("refcount-int64", config.RefCount).
AddField("mediatype", config.MediaType).
AddField("expired-bool", config.Expired).
Noticef("VerifyImage config create")
}
Expand All @@ -58,6 +59,7 @@ func (config VerifyImageConfig) LogModify(logBase *base.LogObject, old interface
oldConfig.Expired != config.Expired {

logObject.CloneAndAddField("refcount-int64", config.RefCount).
AddField("mediatype", config.MediaType).
AddField("expired-bool", config.Expired).
AddField("old-refcount-int64", oldConfig.RefCount).
AddField("old-expired-bool", oldConfig.Expired).
Expand All @@ -74,6 +76,7 @@ func (config VerifyImageConfig) LogDelete(logBase *base.LogObject) {
logObject := base.EnsureLogObject(logBase, base.VerifyImageConfigLogType, config.Name,
nilUUID, config.LogKey())
logObject.CloneAndAddField("refcount-int64", config.RefCount).
AddField("mediatype", config.MediaType).
AddField("expired-bool", config.Expired).
Noticef("VerifyImage config delete")

Expand Down Expand Up @@ -116,6 +119,7 @@ func (status VerifyImageStatus) LogCreate(logBase *base.LogObject) {
return
}
logObject.CloneAndAddField("state", status.State.String()).
AddField("mediatype", status.MediaType).
AddField("refcount-int64", status.RefCount).
AddField("expired-bool", status.Expired).
AddField("size-int64", status.Size).
Expand All @@ -139,6 +143,7 @@ func (status VerifyImageStatus) LogModify(logBase *base.LogObject, old interface
oldStatus.FileLocation != status.FileLocation {

logObject.CloneAndAddField("state", status.State.String()).
AddField("mediatype", status.MediaType).
AddField("refcount-int64", status.RefCount).
AddField("expired-bool", status.Expired).
AddField("size-int64", status.Size).
Expand Down Expand Up @@ -169,6 +174,7 @@ func (status VerifyImageStatus) LogDelete(logBase *base.LogObject) {
logObject := base.EnsureLogObject(logBase, base.VerifyImageStatusLogType, status.Name,
nilUUID, status.LogKey())
logObject.CloneAndAddField("state", status.State.String()).
AddField("mediatype", status.MediaType).
AddField("refcount-int64", status.RefCount).
AddField("expired-bool", status.Expired).
AddField("size-int64", status.Size).
Expand Down
Loading