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.

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

Add timestamp to cluster definition file #710

Closed
corverroos opened this issue Jun 14, 2022 · 3 comments
Closed

Add timestamp to cluster definition file #710

corverroos opened this issue Jun 14, 2022 · 3 comments
Labels
Let's discuss Discussion for general feedback and positive criticism :)

Comments

@corverroos
Copy link
Contributor

corverroos commented Jun 14, 2022

Problem to be solved

It is difficult to look at a cluster definition file (or lock file containing a definition) and know if this is the one you want. It is hard to distinguish multiple versions of lock files with similar config (operator and validator counts).

Proposed solution

Add a timestamp field to cluster_definition so it is clear when the definition was created. It should be a human friendly string field without strict precision requirements, so RFC3339 is good. Ensure it is added to all hash calculations.

Then also add a cluster version bump and add version logic so parsing older versions is supported. This probably entails keeping older version structs and logic around and using version to switch on that.

Out of scope

Adding a timestamp the lock file (outside the definition) is hard since that would need to be included in consensus.

@corverroos corverroos added the Let's discuss Discussion for general feedback and positive criticism :) label Jun 14, 2022
@corverroos corverroos changed the title add timestamps to definition Add timestamp to cluster definition file Jun 14, 2022
@OisinKyne
Copy link
Contributor

OisinKyne commented Jun 14, 2022

So would this be included or excluded from the config hash? Would we want to replace UUID with a nanosecond timestamp? Do we want a human readable timestamp format?

Do we have a free text name/description field still? Should we just by default put a pretty date string into that field if nothing else is specified?

@dB2510
Copy link
Contributor

dB2510 commented Jun 15, 2022

If we are adding timestamp field then it can surely replace existing UUID field. Since this would be constant throughout, it would be included as part of config_hash. It can be in any format I think, although I am more inclined towards unix timestamp format.

@corverroos
Copy link
Contributor Author

corverroos commented Jun 15, 2022

I would say human readable, since the aim is to make a definition/lock file identifiable by looking at it. I want to be able to look at the file and say "ah, yes, this was the one we created during that previous attempt, this other file is older/newer and isn't the one I am searching for".

We can remove UUID if the timestamp is nanosecond granularity.

So suggest RFC3339Nano = "2006-01-02T15:04:05.999999999Z07:00"

It should be included in the config_hash AND definition_hash I think.

I would say timestamp is in addition to a name field. Timestamp is auto generated by the system and always present.

Name is an optional user provided field. If the name is not provided, I would default to a random "adjective-noun" personally so that it can be used to refer to the cluster. "Ah yes, this is the "foo-bar" cluster we created two days ago"/"Which cluster are we running, "foo-bar" or "qux-qux"?

obol-bulldozer bot pushed a commit that referenced this issue Jul 21, 2022
Add human readable `Timestamp` to cluster definition (and therefore cluster lock) and bump version to v1.0.1. This is backwards compatible with v1.0.0 (but not vice versa), see `TestBackwardsCompatability`.

category: feature
ticket: #710
obol-bulldozer bot pushed a commit that referenced this issue Jul 21, 2022
Fixes the latest cluster definition version, it should be v1.1.0 as per feedback in previous PR.

category: bug
ticket: #710
obol-bulldozer bot pushed a commit that referenced this issue Jul 22, 2022
Add a compose smoke test for backwards compatibility with v1.0.0 definition version.

Also enable run version matrix test again.

category: test
ticket: #710
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Let's discuss Discussion for general feedback and positive criticism :)
Projects
None yet
Development

No branches or pull requests

4 participants