-
Notifications
You must be signed in to change notification settings - Fork 547
feat(container-runtime): Add default configuration #24248
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
Conversation
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.
Copilot reviewed 13 out of 15 changed files in this pull request and generated 1 comment.
Files not reviewed (2)
- packages/runtime/container-runtime/package.json: Language not supported
- pnpm-lock.yaml: Language not supported
* For example, use "2.0.0" to set the default configuration for clients running at least the 2.0.0 version | ||
* of the FF runtime. | ||
*/ | ||
readonly compatibilityMode?: string; |
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.
Why not name it more transparently - minVersionRequiredToCollaborate
or something.
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.
I was leaning towards compatibilityMode
to stay consistent with the existing implementation in the declarative model. I'm open to other's opinions on this though
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.
@markfields, I've thought about this a little bit more and I think I still prefer compatibilityMode
for a couple reasons:
- Something like
minVersionRequiredToCollaborate
is a bit misleading IMO. WhatcompatibilityMode
does is set the default configurations for a container runtime based on the inputted version. But if a client withcompatibilityMode
set to "2.0.0" tries to join a session created by a 1.x client, then the 2.x depdenet configs will be disabled by Document Schema and it will still be able to collaborate. - We have not yet implemented a "version control" mechanism that explicity requires version X to collab. For example, let's say
compatibilityMode
is set to "2.40.0" and a client running v2.30.0 tries to join. If there is no difference between the default configurations generated for 2.40/2.30, then they will be able to collaborate without any issue.
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.
But if a client with compatibilityMode set to "2.0.0" tries to join a session created by a 1.x client, then the 2.x depdenet configs will be disabled by Document Schema and it will still be able to collaborate.
Oh, I thought it would be the other way around - The client running 1.x would be kicked out because it doesn't understand the features enabled by the 2.x client.
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.
I would suggest using the word "version" somewhere in the name. "mode" doesn't convey anything meaningful anymore. It made sense when it was a simple number and it explicitly included a set of features. Now, all we are asking is for customers to specify a "version" that they want to be able to collaborate with. What happens behind the scene is something they do not need to care about.
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 other thing to note that this is specifically targetting cross-client compat. So, we should ensure that it is not confused with other compatibility types.
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.
i'm not even sure this should exist on this object, or if using compatibility mode should just produce a IContainerRuntimeOptions. I find it hard to understand the result of setting combability mode, as it really only sets defaults which could be overridden. I think just having some method like, computeCompatibleOptions(mode): IContainerRuntimeOptions, which a consumer could them choose to modify would be much clearer than all the current config mutations.
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.
After thinking about it a bit more, I agree that compatibilityMode
should probably not exist in IContainerRuntimeOptions
. I'm not sure if a method like computeCompatibleOptions
is the way to go, since I think it adds unnecessary steps for the average customer. Personally, I would prefer to keep the approach I took in this PR, but move compatibilityMode
to LoadContainerRuntimeParams
(as an optional param). One reason I like this approach is that it makes the path to making compatibilityMode
a required param easier (if we choose to do so).
I experimented with moving compatibilityMode
to LoadContainerRuntimeParams
in this PR - #24362. If we think this is a good direction I can port the changes to this PR.
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.
I still have quite a few questions about how consumers use this, especially around custom settings for things like mid version rollout. i'd prefer we just have a free function today, and evolve to something else over time, rather than try to do it all at once.
* | ||
* TODO: This should be updated to "2.0.0" when 3.0 is released. | ||
*/ | ||
export const defaultCompatibilityMode = "pre-3.0-default"; |
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.
can we use something semver compatible here, could we just use today's latest version?
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 reason I used a non-semver compatible version here was because I didn't want it to overlap with any actual versions. The goal is to maintain today's default configurations if compatibilityMode
is not defined, but today's default configuratios are inconsistent in terms of what features are enabled/disabled and don't align with any version. For example, there is no such version where batching is enabled, and explicit schema control is disabled. So, the easiest solution IMO is to just use a "placeholder" version until 3.0 is released, then we can use actual semver versions as the default.
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.
now that we use a list of versions below can't we just set the exact version of today to the exact values of today, and use that version as the default version?
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.
I think the same problem still exists. The main issue is that explicitSchemaControl
is disabled by default in today's code. We could potentially hardcode explicitSchemaControl
to be false when no compatMode is provided, but we would still need to remove that logic when 3.0 is released. Since we will need to update defaultCompatibilityMode
when 3.0 is released anyway, I figure it makes sense to have the temporary workarounds be associated with it.
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.
Why can't this just be 2.33.0
and then add entries below to versionDependentOptionConfigMap for 2.33.0
which map for the default values right now. this also gets ride of defaultConfigsForPreFF3
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.
Because if a customer would want to actually use 2.33.0
as their compatMode, then they would get the equivalent of defaultConfigsForPreFF3
which has explicit schema control disabled. If a customer were to use 2.32.0
or 2.34.0
, then explicit schema control would be enabled
// Explicitly disable running Sweep in compat mode "2". Although sweep is supported in 2.x, it is disabled by default. | ||
// This setting explicitly disables it to be extra safe. | ||
minVersionForModernConfig: "3.0.0", | ||
legacyConfig: {}, | ||
modernConfig: { enableGCSweep: true }, |
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.
this is very confusing. compact mode isn't a concept at this layer, it is built above this. 3.0.0 isn't a release version, so why is it necessary for the minVersionForModernConfig to be "3.0.0"?
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.
i think my above comment fixes this: https://github.com/microsoft/FluidFramework/pull/24248/files#r2042841307
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.
@markfields @agarwal-navin helped inform what version to use for minVersionForModernConfig
. In this case it's not "minimum version that can understand this feature", but more "minimum compatibilityMode that we want this feature to be enabled by default for". In this case they felt that we should hold off until it's enabled by default, but we could also choose to change it in the future. Sidenote - I could think about alternative names for minVersionForModernConfig
to make it more clear.
packages/runtime/container-runtime/src/test/compatUtils.spec.ts
Outdated
Show resolved
Hide resolved
packages/runtime/container-runtime/src/test/compatUtils.spec.ts
Outdated
Show resolved
Hide resolved
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output
|
Closing in favor of #24248, may reopen in the future |
…time options (#24379) ## Description This PR adds "default configurations" for container runtime options to the encapsulated model. FF will automatically populate `IContainerRuntimeOptionsInternal` based on the `compatibilityMode` with default values. For example, if `compatibilityMode` is set to `"1.0.0"`, we will automatically disable features that are not compatible with clients running FF runtime v1.0.0 (i.e. grouped batching). Users can still manually define any properties in `IContainerRuntimeOptionsInternal`, which will take precedence over the default configurations. This PR does not yet allow users to input their own `compatibilityMode`, this will be added in a follow up PR. For an example of how it may be implemented, see #24248, which is mostly a superset of the changes in this PR. --------- Co-authored-by: Copilot <[email protected]>
Description
This PR adds "default configurations" to the encapsulated model. To do so, it adds
compatibilityMode
property toIContainerRuntimeOptions
, which takes astring
and represents a valid FF version. FF will automatically apply the proper version-dependent configurations based on the inputtedcompatibilityMode
. For example, ifcompatibilityMode
is set to"1.0.0"
, we will automatically disable features that are not compatible with clients running FF runtime v1.0.0 (i.e. grouped batching).Declarative Model
The main intended audience of this PR are encapsulated model customers, since it adds new capabilities that did not previously exist in the encapsulated model. That said, these changes can also be leveraged by the declarative model as well. This PR modifies the declarative model's implementation of default configurations to leveraging the newly added
compatibilityMode
directly. The benefit is that we no longer need to manually maintaincompatibilityModeRuntimeOptions
, since the options will be automatically generated based on the inputtedcompatibilityMode
. Aditionally, we can/do still add additional configurations to the declarative model (i.e. idCompressor being on by default) when necessary.For declarative model users, there will be no noticeable changes to the public API surface or the behavior when inputting a
compatibilityMode
. The input forcompatibilityMode
remains restricted to"1" | "2"
.Next Steps