Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 19 commits into from
Nov 25, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
995a046
feat(provisioners) - add endpoint to fetch tags associated to a key u…
defelmnq Nov 14, 2024
2ca6c91
rename function
defelmnq Nov 14, 2024
c67d322
linter
defelmnq Nov 14, 2024
911a47d
linter
defelmnq Nov 14, 2024
1a019bb
generate doc
defelmnq Nov 14, 2024
c629f07
changes endpoint to return all pk details
defelmnq Nov 18, 2024
54437f2
generate doc
defelmnq Nov 18, 2024
8eeffb1
work on tests
defelmnq Nov 18, 2024
960084d
Merge remote-tracking branch 'origin/main' into api-endpoint-provisio…
defelmnq Nov 19, 2024
3aa81ed
work on improving tag logs for provisioner start
defelmnq Nov 19, 2024
ebcf687
merge main
defelmnq Nov 20, 2024
0cff661
Work on tags for provisioner daemon
defelmnq Nov 21, 2024
d4472a2
details golint
defelmnq Nov 21, 2024
ee0fef6
change tags variable
defelmnq Nov 25, 2024
703668b
change tags variable
defelmnq Nov 25, 2024
9e0a2d3
Merge remote-tracking branch 'origin/main' into client-provisioners-tags
defelmnq Nov 25, 2024
b5ff465
pre-allocate map
defelmnq Nov 25, 2024
7ea193f
add missing test for failing case
defelmnq Nov 25, 2024
08ab032
improve map allocation
defelmnq Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions enterprise/cli/provisionerdaemonstart.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,22 @@ func (r *RootCmd) provisionerDaemonStart() *serpent.Command {
return err
}

displayedTags := make(map[string]string, len(tags))
if provisionerKey != "" {
pkDetails, err := client.GetProvisionerKey(ctx, provisionerKey)
if err != nil {
return xerrors.New("unable to get provisioner key details")
}

for k, v := range pkDetails.Tags {
displayedTags[k] = v
}
Comment on lines +114 to +116
Copy link
Member

@johnstcn johnstcn Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

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.

} else {
for key, val := range tags {
displayedTags[key] = val
}
}

if name == "" {
name = cliutil.Hostname()
}
Expand Down Expand Up @@ -131,7 +147,7 @@ func (r *RootCmd) provisionerDaemonStart() *serpent.Command {
defer closeLogger()
}

if len(tags) == 0 {
if len(displayedTags) == 0 {
logger.Info(ctx, "note: untagged provisioners can only pick up jobs from untagged templates")
}

Expand Down Expand Up @@ -202,7 +218,7 @@ func (r *RootCmd) provisionerDaemonStart() *serpent.Command {
defer closeFunc()
}

logger.Info(ctx, "starting provisioner daemon", slog.F("tags", tags), slog.F("name", name))
logger.Info(ctx, "starting provisioner daemon", slog.F("tags", displayedTags), slog.F("name", name))

connector := provisionerd.LocalProvisioners{
string(database.ProvisionerTypeTerraform): proto.NewDRPCProvisionerClient(terraformClient),
Expand Down
67 changes: 67 additions & 0 deletions enterprise/cli/provisionerdaemonstart_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,73 @@ func TestProvisionerDaemon_ProvisionerKey(t *testing.T) {
require.Equal(t, proto.CurrentVersion.String(), daemons[0].APIVersion)
})

t.Run("OKWithTags", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

t.Parallel()

ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
defer cancel()
client, user := coderdenttest.New(t, &coderdenttest.Options{
ProvisionerDaemonPSK: "provisionersftw",
LicenseOptions: &coderdenttest.LicenseOptions{
Features: license.Features{
codersdk.FeatureExternalProvisionerDaemons: 1,
codersdk.FeatureMultipleOrganizations: 1,
},
},
})
//nolint:gocritic // ignore This client is operating as the owner user, which has unrestricted permissions
res, err := client.CreateProvisionerKey(ctx, user.OrganizationID, codersdk.CreateProvisionerKeyRequest{
Name: "dont-TEST-me",
Tags: map[string]string{
"tag1": "value1",
"tag2": "value2",
},
})
require.NoError(t, err)
inv, conf := newCLI(t, "provisionerd", "start", "--key", res.Key, "--name=matt-daemon")
err = conf.URL().Write(client.URL.String())
require.NoError(t, err)
pty := ptytest.New(t).Attach(inv)
clitest.Start(t, inv)
pty.ExpectNoMatchBefore(ctx, "check entitlement", "starting provisioner daemon")
pty.ExpectMatchContext(ctx, `tags={"tag1":"value1","tag2":"value2"}`)

var daemons []codersdk.ProvisionerDaemon
require.Eventually(t, func() bool {
daemons, err = client.OrganizationProvisionerDaemons(ctx, user.OrganizationID, nil)
if err != nil {
return false
}
return len(daemons) == 1
}, testutil.WaitShort, testutil.IntervalSlow)
require.Equal(t, "matt-daemon", daemons[0].Name)
require.Equal(t, provisionersdk.ScopeOrganization, daemons[0].Tags[provisionersdk.TagScope])
require.Equal(t, buildinfo.Version(), daemons[0].Version)
require.Equal(t, proto.CurrentVersion.String(), daemons[0].APIVersion)
})

t.Run("NoProvisionerKeyFound", func(t *testing.T) {
t.Parallel()

ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
defer cancel()
client, _ := coderdenttest.New(t, &coderdenttest.Options{
ProvisionerDaemonPSK: "provisionersftw",
LicenseOptions: &coderdenttest.LicenseOptions{
Features: license.Features{
codersdk.FeatureExternalProvisionerDaemons: 1,
codersdk.FeatureMultipleOrganizations: 1,
},
},
})

inv, conf := newCLI(t, "provisionerd", "start", "--key", "ThisKeyDoesNotExist", "--name=matt-daemon")
err := conf.URL().Write(client.URL.String())
require.NoError(t, err)
err = inv.WithContext(ctx).Run()
require.ErrorContains(t, err, "unable to get provisioner key details")
})

t.Run("NoPSK", func(t *testing.T) {
t.Parallel()

Expand Down
Loading