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

Cleanup BlobStoreClientSettings and associated Agent Knob #4696

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions src/Agent.Plugins/Artifact/FileContainerProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ private async Task DownloadFileContainerAsync(IEnumerable<FileContainerItem> ite
{
BlobstoreClientSettings clientSettings = await BlobstoreClientSettings.GetClientSettingsAsync(
connection,
context,
Microsoft.VisualStudio.Services.BlobStore.WebApi.Contracts.Client.BuildArtifact,
tracer,
cancellationToken);
Expand Down
2 changes: 1 addition & 1 deletion src/Agent.Plugins/Artifact/PipelineArtifactServer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ internal async Task UploadAsync(
VssConnection connection = context.VssConnection;
var clientSettings = await BlobstoreClientSettings.GetClientSettingsAsync(
connection,
context,
Microsoft.VisualStudio.Services.BlobStore.WebApi.Contracts.Client.PipelineArtifact,
tracer,
cancellationToken);
Expand All @@ -63,7 +64,6 @@ internal async Task UploadAsync(
DedupManifestArtifactClientFactory.Instance.GetDedupStoreClientMaxParallelism(context),
domainId,
clientSettings,
context,
cancellationToken);

using (clientTelemetry)
Expand Down
2 changes: 1 addition & 1 deletion src/Agent.Sdk/Knob/AgentKnobs.cs
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ public class AgentKnobs
"Enables large chunk size for pipeline artifacts.",
new EnvironmentKnobSource("AGENT_ENABLE_PIPELINEARTIFACT_LARGE_CHUNK_SIZE"),
new RuntimeKnobSource("AGENT_ENABLE_PIPELINEARTIFACT_LARGE_CHUNK_SIZE"),
new BuiltInDefaultKnobSource("false"));
new BuiltInDefaultKnobSource("true")); // This flag is on everywhere and should be on by default now

public static readonly Knob PermissionsCheckFailsafe = new Knob(
nameof(PermissionsCheckFailsafe),
Expand Down
1 change: 1 addition & 0 deletions src/Agent.Worker/Build/FileContainerServer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,7 @@ private async Task<UploadResult> BlobUploadAsync(IAsyncCommandContext context, I

var clientSettings = await BlobstoreClientSettings.GetClientSettingsAsync(
_connection,
context,
Microsoft.VisualStudio.Services.BlobStore.WebApi.Contracts.Client.BuildArtifact,
DedupManifestArtifactClientFactory.CreateArtifactsTracer(verbose, tracer),
token);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ private BlobstoreClientSettings(ClientSettingsInfo settings, IAppTraceSource tra
/// <notes> This should only be called once per client type. This is intended to fail fast so it has no retries.</notes>
public static async Task<BlobstoreClientSettings> GetClientSettingsAsync(
VssConnection connection,
IKnobValueContext context,
BlobStore.WebApi.Contracts.Client? client,
IAppTraceSource tracer,
CancellationToken cancellationToken)
Expand All @@ -50,14 +51,26 @@ public static async Task<BlobstoreClientSettings> GetClientSettingsAsync(

var blobUri = connection.GetClient<ClientSettingsHttpClient>().BaseAddress;
var clientSettingsHttpClient = factory.CreateVssHttpClient<IClientSettingsHttpClient, ClientSettingsHttpClient>(blobUri);
return new BlobstoreClientSettings(await clientSettingsHttpClient.GetSettingsAsync(client.Value, userState: null, cancellationToken), tracer);
var clientSettings = await clientSettingsHttpClient.GetSettingsAsync(client.Value, userState: null, cancellationToken);

// Have to keep this check in for pipelines that have manually opted out for pipeline artifacts:
if (client == BlobStore.WebApi.Contracts.Client.PipelineArtifact && !AgentKnobs.AgentEnablePipelineArtifactLargeChunkSize.GetValue(context).AsBoolean())
{
if (clientSettings != null && clientSettings.Properties.ContainsKey(ClientSettingsConstants.ChunkSize))
{
clientSettings.Properties.Remove(ClientSettingsConstants.ChunkSize);
}
}

return new BlobstoreClientSettings(clientSettings, tracer);
}
catch (Exception exception)
{
// Use info cause we don't want to fail builds with warnings as errors...
tracer.Info($"Error while retrieving client Settings for {client}. Exception: {exception}. Falling back to defaults.");
}
}

return new BlobstoreClientSettings(null, tracer);
}

Expand All @@ -83,23 +96,19 @@ public IDomainId GetDefaultDomainId()
return domainId;
}

public HashType GetClientHashType(AgentTaskPluginExecutionContext context)
public HashType GetClientHashType()
{
HashType hashType = ChunkerHelper.DefaultChunkHashType;

// Note: 9/6/2023 Remove the below check in couple of months.
if (AgentKnobs.AgentEnablePipelineArtifactLargeChunkSize.GetValue(context).AsBoolean())
if (clientSettings != null && clientSettings.Properties.ContainsKey(ClientSettingsConstants.ChunkSize))
{
if (clientSettings != null && clientSettings.Properties.ContainsKey(ClientSettingsConstants.ChunkSize))
try
{
try
{
HashTypeExtensions.Deserialize(clientSettings.Properties[ClientSettingsConstants.ChunkSize], out hashType);
}
catch (Exception exception)
{
tracer.Info($"Error converting the chunk size '{clientSettings.Properties[ClientSettingsConstants.ChunkSize]}': {exception.Message}. Falling back to default.");
}
HashTypeExtensions.Deserialize(clientSettings.Properties[ClientSettingsConstants.ChunkSize], out hashType);
}
catch (Exception exception)
{
tracer.Info($"Error converting the chunk size '{clientSettings.Properties[ClientSettingsConstants.ChunkSize]}': {exception.Message}. Falling back to default.");
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ public interface IDedupManifestArtifactClientFactory
int maxParallelism,
IDomainId domainId,
BlobstoreClientSettings clientSettings,
AgentTaskPluginExecutionContext context,
CancellationToken cancellationToken);

/// <summary>
Expand Down Expand Up @@ -112,6 +111,7 @@ private DedupManifestArtifactClientFactory()
{
var clientSettings = await BlobstoreClientSettings.GetClientSettingsAsync(
connection,
context,
client,
CreateArtifactsTracer(verbose, traceOutput),
cancellationToken);
Expand All @@ -123,7 +123,6 @@ private DedupManifestArtifactClientFactory()
DedupManifestArtifactClientFactory.Instance.GetDedupStoreClientMaxParallelism(context),
domainId,
clientSettings,
context,
cancellationToken);
}

Expand All @@ -134,7 +133,6 @@ private DedupManifestArtifactClientFactory()
int maxParallelism,
IDomainId domainId,
BlobstoreClientSettings clientSettings,
AgentTaskPluginExecutionContext context,
CancellationToken cancellationToken)
{
const int maxRetries = 5;
Expand All @@ -150,7 +148,8 @@ private DedupManifestArtifactClientFactory()
IDedupStoreHttpClient dedupStoreHttpClient = GetDedupStoreHttpClient(connection, domainId, maxRetries, tracer, cancellationToken);

var telemetry = new BlobStoreClientTelemetry(tracer, dedupStoreHttpClient.BaseAddress);
HashType hashType= clientSettings.GetClientHashType(context);

HashType hashType = clientSettings.GetClientHashType();

if (hashType == BuildXL.Cache.ContentStore.Hashing.HashType.Dedup1024K)
{
Expand All @@ -164,7 +163,12 @@ private DedupManifestArtifactClientFactory()
return (new DedupManifestArtifactClient(telemetry, dedupClient, tracer), telemetry);
}

private static IDedupStoreHttpClient GetDedupStoreHttpClient(VssConnection connection, IDomainId domainId, int maxRetries, IAppTraceSource tracer, CancellationToken cancellationToken)
private static IDedupStoreHttpClient GetDedupStoreHttpClient(
VssConnection connection,
IDomainId domainId,
int maxRetries,
IAppTraceSource tracer,
CancellationToken cancellationToken)
{
ArtifactHttpClientFactory factory = new ArtifactHttpClientFactory(
connection.Credentials,
Expand Down Expand Up @@ -196,6 +200,7 @@ private static IDedupStoreHttpClient GetDedupStoreHttpClient(VssConnection conne
});
return dedupStoreHttpClient;
}

public (DedupStoreClient client, BlobStoreClientTelemetryTfs telemetry) CreateDedupClient(
VssConnection connection,
IDomainId domainId,
Expand Down
1 change: 1 addition & 0 deletions src/Microsoft.VisualStudio.Services.Agent/JobServer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ public async Task<BlobIdentifierWithBlocks> UploadLogToBlobStore(Stream blob, st
int maxParallelism = HostContext.GetService<IConfigurationStore>().GetSettings().MaxDedupParallelism;
var clientSettings = await BlobstoreClientSettings.GetClientSettingsAsync(
_connection,
context: null,
client: null,
DedupManifestArtifactClientFactory.CreateArtifactsTracer(verbose, (str) => Trace.Info(str)), cancellationToken);
var (dedupClient, clientTelemetry) = DedupManifestArtifactClientFactory.Instance
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ public class MockDedupManifestArtifactClientFactory : IDedupManifestArtifactClie
int maxParallelism,
IDomainId domainId,
BlobstoreClientSettings clientSettings,
AgentTaskPluginExecutionContext context,
CancellationToken cancellationToken)
{
telemetrySender = new TestTelemetrySender();
Expand Down
Loading