set MediaType in BlobStatus from VerifyImageStatus #4511
Conversation
The MediaType field should not be empty because this can affect correct function of IsManifest() and IsIndex(). This commit additionally logs MediaType in all pubsub structs to facilitate future debugging. Signed-off-by: Paul Gaiduk <paulg@zededa.com>
europaul
requested review from
OhmSpectator,
deitch,
rouming and
eriknordmark
as code owners
| @@ -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). | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Tbh I have no idea why it's there. And some fields like state don't follow it.
| @@ -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, | |||
There was a problem hiding this comment.
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
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.
There was a problem hiding this comment.
That is the hardest part. This seems to make sense to me, but the interplay between them is hard to understand.
There was a problem hiding this comment.
I agree with both of you. But I also don't see any harm in setting it here.
There was a problem hiding this comment.
do we have any test to reproduce the bug / verify that the fix is working?
There was a problem hiding this comment.
@OhmSpectator which test does this refer to?
There was a problem hiding this comment.
https://github.com/zededa/bug-reproduction/tree/main/ci-197 here it is, but it requires some extra setup, if I remember correctly
There was a problem hiding this comment.
@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...
This change is an attempt to fix #4510.
This doesn't necessarily fix the bug, but should facilitate further debugging by adding more relevant logs.
Also it sets the
MediaTypefield ofBlobStatuswhen created fromVerifyImageStatus.