fix: improve log on provisioner daemon started with pk #15588

defelmnq · 2024-11-19T09:31:57Z

This PR aims to fetch the provisioned key details when starting a provisioned daemon - for now in order to access the tags associated to the provisioned key and display them accordingly in the starting logs.

We do not want to change any other logic inside this PR as it was already working as expected.

…sing its id fix: change from database.StringMap to codersdk.ProvisionerKeyTags in endpoint response improve annotations for endpoint move logic to enterprise part generate new doc move logic to enterprise part generate doc

…ners-info

johnstcn · 2024-11-19T15:41:57Z

cc @johnstcn I tried to think about how to test this one but am not sure to find something that makes sense.

Take a look at the existing tests in enterprise/cli/provisionerdaemonstart_test.go: in particular, tests that leverage ptytest.New(inv) to assert the stdin/stdout of the command that's being invoked.

johnstcn · 2024-11-19T15:42:14Z

I assume that #15505 covers part of this PR?

defelmnq · 2024-11-19T16:11:35Z

Yeah indeed @johnstcn that's why I kept it as a draft until the other one is merged - was more to be able to validate CI & ensure its all ready.

About the tests tips thanks, currently checking.

johnstcn · 2024-11-21T15:58:45Z

enterprise/cli/provisionerdaemonstart.go

+				for k, v := range pkDetails.Tags {
+					displayedTags[k] = v
+				}


I think we can overwrite displayedTags here entirely. If using a provisioner key, none of the tags specified via tags will be used to match jobs to the provisioner. (In fact, I don't think you're even allowed specify both --key and --tag)

I preferred to keep two different variables as otherwise it would require some other logic changes.

The tag variable is used to call the /provisionerdaemon/serve endpoint below, and using the same variable would mean changes here too, which I feel like we dont want.

wdyt ?

For the sake of correctness, I'd like to make this change.
Otherwise this may confuse future readers of the code.
However, let's do it as a separate follow-up PR.

defelmnq · 2024-11-21T15:59:07Z

enterprise/cli/provisionerdaemonstart.go

@@ -104,6 +104,22 @@ func (r *RootCmd) provisionerDaemonStart() *serpent.Command {
 				return err
 			}

+			displayedTags := make(map[string]string)


review: This displayedTags var is merging both tags provided in the CLI and tags fetched from the provisionerKey.

enterprise/cli/provisionerdaemonstart.go

dannykopping · 2024-11-21T15:56:44Z

enterprise/cli/provisionerdaemonstart_test.go

@@ -294,6 +294,51 @@ func TestProvisionerDaemon_ProvisionerKey(t *testing.T) {
 		require.Equal(t, proto.CurrentVersion.String(), daemons[0].APIVersion)
 	})

+	t.Run("OKWithTags", func(t *testing.T) {


Are there any sad-cases that we could be testing here?
Is there a test already (sorry, being lazy) which already checks that if a provisioner key is not defined then it will not display the tags?

We could add a test to validate what happen in case of the client.GetProvisionerKey failing - I've just done it.

Otherwise the logic should not be impacted and the endpoint itself seems pretty much well tested.

I'm not really understanding the point of NoProvisionerKeyFound. The purpose of this PR is to add changes to tags which are logged, but AFAICS you're not looking at the outputted logs? Does the provisioner fail to start in this case?

Yeah that's exactly it - I am open to the behavior we want to apply here but globally if we can not contact the backend / retrieve the key , I returned and error and did not continued to start the provisioner.

We can instead just output an error and continue as if nothing happened - but the log will be the same as of now (without the tags)

Cool, no need to change the behaviour - I was just confirming what the intent was for this test.

dannykopping · 2024-11-25T06:53:24Z

enterprise/cli/provisionerdaemonstart.go

+					return xerrors.New("unable to get provisioner key details")
+				}
+
+				displayedTags = make(map[string]string, len(pkDetails.Tags))


What is your intention here? To clear out the map if the provisioner key is set?

So globally, either we have no key and some tags (line 107 to 110) - either we have no key (line 112) and want to fill the displayedTags with the ones fetched in pkDetails.

That's why at first I was using the same map without allocating it , as we can't really be sure about the size before knowing in which case we'll enter
anyway, we want the map to be declared in the whole function to be used below.

Seems like you should add an else block which populates displayedTags if the provision key is nil?

I don't think what you have is a big deal performance-wise since maps can never shrink, so if the make call on L107 has a length of 4 and the call on L118 has a length of 2, it'll keep the originally-allocated memory.
Preallocation is not strictly necessary but it's a good practice. In this case if there's a chance we'll allocate twice then there's not much of a saving since that's what preallocation is trying to prevent.

tried to improve it from your feedback, was it what you had in mind ? 🙏

I basically allocate before the logic, then fill it with the tags i want based on the case - worst situation, it will reallocate with more space for the tags fetched if required.

dannykopping · 2024-11-25T06:55:20Z

enterprise/cli/provisionerdaemonstart_test.go

@@ -294,6 +294,51 @@ func TestProvisionerDaemon_ProvisionerKey(t *testing.T) {
 		require.Equal(t, proto.CurrentVersion.String(), daemons[0].APIVersion)
 	})

+	t.Run("OKWithTags", func(t *testing.T) {


I'm not really understanding the point of NoProvisionerKeyFound. The purpose of this PR is to add changes to tags which are logged, but AFAICS you're not looking at the outputted logs? Does the provisioner fail to start in this case?

dannykopping

LGTM

dannykopping · 2024-11-25T07:50:55Z

enterprise/cli/provisionerdaemonstart.go

+					return xerrors.New("unable to get provisioner key details")
+				}
+
+				displayedTags = make(map[string]string, len(pkDetails.Tags))


Seems like you should add an else block which populates displayedTags if the provision key is nil?

I don't think what you have is a big deal performance-wise since maps can never shrink, so if the make call on L107 has a length of 4 and the call on L118 has a length of 2, it'll keep the originally-allocated memory.
Preallocation is not strictly necessary but it's a good practice. In this case if there's a chance we'll allocate twice then there's not much of a saving since that's what preallocation is trying to prevent.

defelmnq added 10 commits November 14, 2024 08:02

rename function

2ca6c91

linter

c67d322

linter

911a47d

generate doc

1a019bb

changes endpoint to return all pk details

c629f07

generate doc

54437f2

work on tests

8eeffb1

Merge remote-tracking branch 'origin/main' into api-endpoint-provisio…

960084d

…ners-info

work on improving tag logs for provisioner start

3aa81ed

github-actions bot assigned defelmnq Nov 19, 2024

defelmnq changed the title ~~fix(cli): improve log on provisioner daemon started with pk~~ fix: improve log on provisioner daemon started with pk Nov 19, 2024

defelmnq added 2 commits November 20, 2024 22:13

merge main

ebcf687

Work on tags for provisioner daemon

0cff661

defelmnq marked this pull request as ready for review November 21, 2024 15:52

defelmnq requested review from dannykopping and johnstcn November 21, 2024 15:52

johnstcn reviewed Nov 21, 2024

View reviewed changes

defelmnq commented Nov 21, 2024

View reviewed changes

dannykopping reviewed Nov 21, 2024

View reviewed changes

defelmnq added 6 commits November 21, 2024 22:54

details golint

d4472a2

change tags variable

ee0fef6

change tags variable

703668b

Merge remote-tracking branch 'origin/main' into client-provisioners-tags

9e0a2d3

pre-allocate map

b5ff465

add missing test for failing case

7ea193f

defelmnq requested a review from dannykopping November 25, 2024 06:48

defelmnq requested a review from johnstcn November 25, 2024 06:49

dannykopping reviewed Nov 25, 2024

View reviewed changes

dannykopping approved these changes Nov 25, 2024

View reviewed changes

improve map allocation

08ab032

johnstcn approved these changes Nov 25, 2024

View reviewed changes

defelmnq merged commit a8becfb into main Nov 25, 2024
27 checks passed

defelmnq deleted the client-provisioners-tags branch November 25, 2024 09:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: improve log on provisioner daemon started with pk #15588

fix: improve log on provisioner daemon started with pk #15588

defelmnq commented Nov 19, 2024 •

edited

Loading

johnstcn commented Nov 19, 2024

johnstcn commented Nov 19, 2024

defelmnq commented Nov 19, 2024

johnstcn Nov 21, 2024 •

edited

Loading

defelmnq Nov 25, 2024

johnstcn Nov 25, 2024

defelmnq Nov 21, 2024

dannykopping Nov 21, 2024

defelmnq Nov 25, 2024

dannykopping Nov 25, 2024

defelmnq Nov 25, 2024

dannykopping Nov 25, 2024

dannykopping Nov 25, 2024

defelmnq Nov 25, 2024

dannykopping Nov 25, 2024

defelmnq Nov 25, 2024 •

edited

Loading

dannykopping Nov 25, 2024

dannykopping left a comment

dannykopping Nov 25, 2024

fix: improve log on provisioner daemon started with pk #15588

fix: improve log on provisioner daemon started with pk #15588

Conversation

defelmnq commented Nov 19, 2024 • edited Loading

johnstcn commented Nov 19, 2024

johnstcn commented Nov 19, 2024

defelmnq commented Nov 19, 2024

johnstcn Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

defelmnq Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannykopping left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

defelmnq commented Nov 19, 2024 •

edited

Loading

johnstcn Nov 21, 2024 •

edited

Loading

defelmnq Nov 25, 2024 •

edited

Loading