-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Ignore timestamp labels during sorting and release of images #2594
Conversation
Can you add a test which would had showed the problem this PR is fixing? |
@2opremio I will try to find some time. |
@2opremio the problem is fairly simple, labels should always be ignored and only be used by the cache decorator in case an image has been whitelisted to replace the |
Info.CreatedTS
methodIn #2176 a whitelist option for the usage of image timestamps from labels was implemented. This PR did however not remove the `CreatedTS` method, with as a result that some elements of Flux like `fluxctl` would still consult the timestamp parsed from labels, rather than relying on the cache decorated `info.CreatedAt`. This commit removes the method, and replaces any calls made to it with `info.CreatedAt`, so that a timestamp from a label is not mistakenly used while the image has not been whitelisted.
0732dd5
to
dd2f2c4
Compare
Hey @hiddeco -- Sorry to bother but I have a question regarding this change. We recently upgraded from flux version 1.12.3 to 1.16.0. We are receiving the following error because our timestamp label does not follow RFC3339:
and also:
We have several images that are producing this error which stops them from being deployed by Flux. We are trying to fix them but this is blocking us from upgrading flux quite a bit and fixing them all will take a while. This change looks like it's for fluxctl but I thought I'd double check I'm not overlooking something. Is there some sort of flag and/or annotation we can add to fluxctl to turn this behavior off? Thanks a lot! |
@AMoghrabi the former log message is just a mere warning, it has no impact on Flux' automation. The latter log message is however a problem for Flux, as it needs the timestamps from all images to be able to perform automation operations. If the image has a creation timestamp but Flux says it doesn't, can you please try clearing the image metadata cache (scale up/down memcached) to see if this resolves the issue? |
Thanks a lot for your response @hiddeco. I've deleting the memcached pod and haven't had any luck. I could try scaling it that would produce different results (though I don't think that's the case). I've checked another service that is having the same issue. Inspecting the image which is reported to have a zero-timestamp, I see this:
But in an issue reported on this repo, one of the developers mentioned Flux does not look at Inspecting the labels we add to the image during a docker build, I see this:
I think this is what Flux is complaining about as it isn't following the RFC spec, and maybe Flux does not know how to parse and read this format so it results in a zero-timestamp. Does that sound about right? Otherwise, on the image I also see this field: It seems like our only option is to fix these timestamps publish a new image. Afterwards, we need to manually redeploy the image without the help of Flux as Flux would continue to struggle, as the currently running image does not have a proper timestamp and cannot do a comparison. What are your thoughts? Let me know if you'd like me to open an issue instead of going forward in this PR. Thanks again! Edit: I forgot to check... I think this particular service may still be able to deploy, I haven't looked into it. I think it causes a problem when we update the timestamp label to something that conforms to the RFC spec. But I'll need to test that still. I'l open to any ideas you may have though, thanks. |
In #2176 a whitelist option for the usage of image timestamps from
labels was implemented. This PR did however not remove the
CreatedTS
method, with as a result that some elements of Flux likefluxctl
would still consult the timestamp parsed from labels, ratherthan relying on the cache decorated
info.CreatedAt
.This commit removes the method, and replaces any calls made to it
with
info.CreatedAt
, so that a timestamp from a label is notmistakenly used while the image has not been whitelisted.