-
Notifications
You must be signed in to change notification settings - Fork 96
feat: add telemetry as part of mount options for blobfuse mounts #2248
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
base: master
Are you sure you want to change the base?
Changes from all commits
7999704
2b4e9c8
8d9b9fd
5c45e5b
618aac5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -34,9 +34,13 @@ import ( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mutex sync.Mutex | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mutex sync.Mutex | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| driverVersion string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // telemetryTagPrefix is used to identify the mounts done via blobcsi driver | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const telemetryTagPrefix = "blobpartner-csi/" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type BlobfuseVersion int | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -47,12 +51,14 @@ const ( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type MountServer struct { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| blobfuseVersion BlobfuseVersion | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mount_azure_blob.UnimplementedMountServiceServer | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| exec func(name string, arg ...string) *exec.Cmd | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // NewMountServer returns a new Mountserver | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func NewMountServiceServer() *MountServer { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mountServer := &MountServer{} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mountServer.blobfuseVersion = getBlobfuseVersion() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mountServer.exec = exec.Command | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return mountServer | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -71,6 +77,7 @@ func (server *MountServer) MountAzureBlob(_ context.Context, | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var cmd *exec.Cmd | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var result mount_azure_blob.MountAzureBlobResponse | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if protocol == blob.Fuse2 || server.blobfuseVersion == BlobfuseV2 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| telemetryTag := telemetryTagPrefix + driverVersion | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| args = "mount " + args | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // add this arg for blobfuse2 to solve the issue: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // https://github.com/Azure/azure-storage-fuse/issues/1015 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -82,13 +89,28 @@ func (server *MountServer) MountAzureBlob(_ context.Context, | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| klog.V(2).Infof("append --disable-version-check to mount args") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| args = args + " " + "--disable-version-check=true" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Adding telemetry tag to know that blob is been mounted through AKS CSI Driver | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if !strings.Contains(args, "--telemetry") { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| klog.V(2).Infof("append --telemetry=%s to mount args", telemetryTag) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| args = args + " " + "--telemetry=" + telemetryTag | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // If telemetry flag is already present, check for aks tag if not present | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If telemetry flag is already present, we just respect original setting, I don't think we need to change the original value by force
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If user has some custom tag, then we can't use that for tracking blob usage through AKS. If we just use the original setting then whole purpose to track usage will be lost, so I appended our tag with user specified value.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but what if customer only wants to keep the original telemetry? this PR would change the telemetry even user does not want to. and this adds the telemetry as
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have updated tag to |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // then user might have their own telemetry tag append aks tag to it | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if !strings.Contains(args, telemetryTagPrefix) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| splitedArgs := strings.Split(args, "--telemetry=") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if len(splitedArgs) == 2 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| args = splitedArgs[0] + " --telemetry=" + telemetryTag + "," + splitedArgs[1] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
mittachaitu marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+100
to
+102
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| splitedArgs := strings.Split(args, "--telemetry=") | |
| if len(splitedArgs) == 2 { | |
| args = splitedArgs[0] + " --telemetry=" + telemetryTag + "," + splitedArgs[1] | |
| splitArgs := strings.Split(args, "--telemetry=") | |
| if len(splitArgs) == 2 { | |
| args = splitArgs[0] + " --telemetry=" + telemetryTag + "," + splitArgs[1] |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string manipulation logic for inserting the telemetry tag creates a potential issue with spacing. The code constructs splitedArgs[0] + " --telemetry=" + telemetryTag + "," + splitedArgs[1], which adds a space before --telemetry= but doesn't trim it from splitedArgs[0]. This could result in double spaces if splitedArgs[0] already ends with a space. Consider using strings.TrimSpace() on splitedArgs[0] before concatenation.
| args = splitedArgs[0] + " --telemetry=" + telemetryTag + "," + splitedArgs[1] | |
| args = strings.TrimSpace(splitedArgs[0]) + " --telemetry=" + telemetryTag + "," + splitedArgs[1] |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic doesn't handle the edge case where --telemetry= is provided with an empty value (e.g., --telemetry= --other-flag). In this case, splitedArgs[1] would start with a space and a different flag, resulting in malformed telemetry arguments like --telemetry=blobpartner-csi/version, --other-flag. Consider trimming whitespace from splitedArgs[1] and checking if it's empty before concatenating.
| args = splitedArgs[0] + " --telemetry=" + telemetryTag + "," + splitedArgs[1] | |
| telemetryValue := strings.TrimSpace(splitedArgs[1]) | |
| if telemetryValue == "" { | |
| args = splitedArgs[0] + " --telemetry=" + telemetryTag | |
| } else { | |
| args = splitedArgs[0] + " --telemetry=" + telemetryTag + "," + telemetryValue | |
| } |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The telemetry string parsing logic has a critical flaw. When splitting by "--telemetry=", if there are multiple occurrences of this substring in args (e.g., in a container name or account name), len(splitedArgs) could be greater than 2, but only the first occurrence is handled. This could result in malformed arguments. Consider using a more robust parsing method such as regular expressions or proper argument parsing to specifically target the --telemetry flag.
| splitedArgs := strings.Split(args, "--telemetry=") | |
| if len(splitedArgs) == 2 { | |
| args = splitedArgs[0] + " --telemetry=" + telemetryTag + "," + splitedArgs[1] | |
| } | |
| klog.V(2).Infof("updated --telemetry tag in mount args: %s", args) | |
| // Split args into slice for robust parsing | |
| argSlice := strings.Fields(args) | |
| telemetryIdx := -1 | |
| for i, arg := range argSlice { | |
| if strings.HasPrefix(arg, "--telemetry=") { | |
| telemetryIdx = i | |
| break | |
| } | |
| } | |
| if telemetryIdx != -1 { | |
| // Update the telemetry tag, appending our tag if not present | |
| telemetryVal := strings.TrimPrefix(argSlice[telemetryIdx], "--telemetry=") | |
| // Avoid duplicate tags | |
| if !strings.Contains(telemetryVal, telemetryTagPrefix) { | |
| argSlice[telemetryIdx] = "--telemetry=" + telemetryTag + "," + telemetryVal | |
| args = strings.Join(argSlice, " ") | |
| klog.V(2).Infof("updated --telemetry tag in mount args: %s", args) | |
| } | |
| } |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code silently does nothing when len(splitedArgs) != 2 (line 101 condition). This can occur if the string "--telemetry=" appears multiple times or is part of another parameter value. Consider adding logging (e.g., klog.Warningf) to indicate when the telemetry tag cannot be updated, so operators can identify potential issues with their mount arguments.
| } | |
| klog.V(2).Infof("updated --telemetry tag in mount args: %s", args) | |
| klog.V(2).Infof("updated --telemetry tag in mount args: %s", args) | |
| } else { | |
| klog.Warningf("could not update --telemetry tag in mount args: unexpected format or multiple --telemetry= flags found in args: %s", args) | |
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,8 @@ package server | |
|
|
||
| import ( | ||
| "context" | ||
| "os/exec" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
|
|
@@ -64,3 +66,95 @@ func TestServerMountAzureBlob(t *testing.T) { | |
| }) | ||
| } | ||
| } | ||
|
|
||
| // fakeExecCommand is used to mock exec.Command for testing, it returns list of args | ||
| func fakeExecCommandEchoArgs(_ string, args ...string) *exec.Cmd { | ||
| return exec.Command("echo", append([]string{"-n"}, args...)...) | ||
| } | ||
|
|
||
| func TestServerMountAzureBlob_Telemetry(t *testing.T) { | ||
| driverVersion = "fake-version" | ||
| t.Parallel() | ||
| testCases := []struct { | ||
| name string | ||
| args string | ||
| code codes.Code | ||
| mountServer MountServer | ||
| areValidTelemetryArgs func(cmdArgs string) bool | ||
| }{ | ||
| { | ||
| name: "mount_with_telemetry_tag_blobfusev2", | ||
| args: "--account-name=testaccount --container-name=testcontainer --telemetry=volume1-app1 --tmp-path=/tmp/blobfuse-tmp", | ||
| mountServer: MountServer{ | ||
| blobfuseVersion: BlobfuseV2, | ||
| exec: fakeExecCommandEchoArgs, | ||
| }, | ||
| code: codes.OK, | ||
| areValidTelemetryArgs: func(cmdArgs string) bool { | ||
| expectedTelemetryArg := "--telemetry=" + telemetryTagPrefix + driverVersion + ",volume1-app1" | ||
| return strings.Contains(cmdArgs, expectedTelemetryArg) | ||
| }, | ||
| }, | ||
| { | ||
| name: "mount_without_telemetry_tag_blobfusev2", | ||
| args: "--account-name=testaccount --container-name=testcontainer --tmp-path=/tmp/blobfuse-tmp", | ||
| mountServer: MountServer{ | ||
| blobfuseVersion: BlobfuseV2, | ||
| exec: fakeExecCommandEchoArgs, | ||
| }, | ||
| code: codes.OK, | ||
| areValidTelemetryArgs: func(cmdArgs string) bool { | ||
| expectedTelemetryArg := "--telemetry=" + telemetryTagPrefix + driverVersion | ||
| return strings.Contains(cmdArgs, expectedTelemetryArg) | ||
| }, | ||
| }, | ||
| { | ||
| name: "mount_with_same_telemetry_tag_blobfusev2", | ||
| args: "--account-name=testaccount --container-name=testcontainer --telemetry=" + telemetryTagPrefix + driverVersion, | ||
| mountServer: MountServer{ | ||
| blobfuseVersion: BlobfuseV2, | ||
| exec: fakeExecCommandEchoArgs, | ||
| }, | ||
| code: codes.OK, | ||
| areValidTelemetryArgs: func(cmdArgs string) bool { | ||
| // Argument order should remain unchanged | ||
| return strings.Contains(cmdArgs, "--account-name=testaccount --container-name=testcontainer --telemetry="+telemetryTagPrefix+driverVersion) | ||
| }, | ||
| }, | ||
| { | ||
| name: "mount_with_blobfusev1", | ||
| args: "--account-name=testaccount --container-name=testcontainer --tmp-path=/tmp/blobfuse-tmp", | ||
| mountServer: MountServer{ | ||
| blobfuseVersion: BlobfuseV1, | ||
| exec: fakeExecCommandEchoArgs, | ||
| }, | ||
| code: codes.OK, | ||
| areValidTelemetryArgs: func(cmdArgs string) bool { | ||
| // No telemetry arg should be added for blobfuse v1 | ||
| return !strings.Contains(cmdArgs, "--telemetry=") && cmdArgs == "--account-name=testaccount --container-name=testcontainer --tmp-path=/tmp/blobfuse-tmp" | ||
| }, | ||
| }, | ||
|
Comment on lines
+100
to
+136
|
||
| } | ||
|
|
||
| for i := range testCases { | ||
| tc := testCases[i] | ||
|
|
||
| t.Run(tc.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| req := mount_azure_blob.MountAzureBlobRequest{ | ||
| MountArgs: tc.args, | ||
| AuthEnv: []string{}, | ||
| } | ||
| res, err := tc.mountServer.MountAzureBlob(context.Background(), &req) | ||
| if tc.code == codes.OK { | ||
| require.NoError(t, err) | ||
| require.NotNil(t, res) | ||
| require.True(t, tc.areValidTelemetryArgs(res.Output), "telemetry args are not valid in command args: %s", res.Output) | ||
| } else { | ||
| require.Error(t, err) | ||
| require.NotNil(t, res) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The telemetry tag prefix is inconsistent with the PR description. The PR description states the telemetry tag should be "azpartner-aks/", but the implementation uses "blobpartner-csi/". This mismatch needs to be corrected to match the intended telemetry format.