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

docs: add config documentation #155

Merged
merged 1 commit into from
May 9, 2024

Conversation

sergelogvinov
Copy link
Collaborator

Update documentation.

Pull Request

What? (description)

Why? (reasoning)

Acceptance

Please use the following checklist:

  • you linked an issue (if applicable)
  • you included tests (if applicable)
  • you ran conformance (make conformance)
  • you linted your code (make lint)
  • you linted your code (make unit)

See make help for a description of the available targets.

@sergelogvinov
Copy link
Collaborator Author

Hi @rothgar can you check this documentation.
Is it understandable?

Thanks.

@sergelogvinov sergelogvinov requested a review from rothgar May 3, 2024 10:07
docs/config.md Outdated
annotations:
...
# Set annotations based on transformation rules
talos.dev/instance-id: "id-e8e8c388-5812-4db0-87e2-ad1fee51a1c1"
Copy link
Member

Choose a reason for hiding this comment

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

Should there also be an omni instance-id or will they be the same?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is just a key/value string, You can set everything that you want. In this example - talos.dev/instance-id: "id-{{ .InstanceID }}" there InstanceID - is cloud instance ID

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 my main question is if this should be talos.dev/ or an omni identifier. Will the ID be the same in this label as it shows up in Omni?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh i see, i renamed it, to not confuse people ;) custom-annotation/instance-id

docs/config.md Show resolved Hide resolved
InstanceType string `yaml:"instanceType,omitempty" protobuf:"5"`
InstanceID string `yaml:"instanceId,omitempty" protobuf:"6"`
ProviderID string `yaml:"providerId,omitempty" protobuf:"7"`
Spot bool `yaml:"spot,omitempty" protobuf:"8"`
Copy link
Member

Choose a reason for hiding this comment

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

In AWS this is called "provisioning type" or "provisioning model" (GCP) and would be a string like spot, on-demand, or reserved.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Spot, if true - can terminate at any time (cloud decision). on-demand and reserved - your decision

Copy link
Member

Choose a reason for hiding this comment

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

Is the goal of this field to set the controller to do specific things with spot instances only?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do not deploy pods which cannot migrate fast (long task jobs for instance)

Copy link
Member

Choose a reason for hiding this comment

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

If our goal is to help users not run jobs that cannot terminate quickly I think spot: true/false is to vague. There are other reasons they may want to target spot, on-demand, and reserved instance types. And not every cloud provider calls spare capacity "spot".

Karpenter uses the capacity-type label which I think would be a more generic way to present this information that works across different cloud environments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep ~2 years ago, Karpenter was not so popular.

Make cense to rename it in future.

Copy link
Member

Choose a reason for hiding this comment

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

OK, no problem. As long as we can change it it the future if we need to.

Copy link
Collaborator Author

@sergelogvinov sergelogvinov left a comment

Choose a reason for hiding this comment

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

I'll add more details.

Thanks.

docs/config.md Outdated
annotations:
...
# Set annotations based on transformation rules
talos.dev/instance-id: "id-e8e8c388-5812-4db0-87e2-ad1fee51a1c1"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is just a key/value string, You can set everything that you want. In this example - talos.dev/instance-id: "id-{{ .InstanceID }}" there InstanceID - is cloud instance ID

docs/config.md Show resolved Hide resolved
InstanceType string `yaml:"instanceType,omitempty" protobuf:"5"`
InstanceID string `yaml:"instanceId,omitempty" protobuf:"6"`
ProviderID string `yaml:"providerId,omitempty" protobuf:"7"`
Spot bool `yaml:"spot,omitempty" protobuf:"8"`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Spot, if true - can terminate at any time (cloud decision). on-demand and reserved - your decision

Update documentation.

Signed-off-by: Serge Logvinov <[email protected]>
@sergelogvinov
Copy link
Collaborator Author

/m

@talos-bot talos-bot merged commit c0988a3 into siderolabs:main May 9, 2024
2 checks passed
@sergelogvinov sergelogvinov deleted the docs-config branch May 9, 2024 07:05
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.

3 participants