-
Notifications
You must be signed in to change notification settings - Fork 226
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
OCPBUGS-44849: Provide fallback Xattr method where not supported in k…
…ernel.
Some customers are expereincing breaking issues when their OS does not support xattr, most notably NFS + Xattr
This causes installations not to proceed and needs to be addressed urgently.
This change provides a means of 'falling back' to a filesystem based implementation in the case that xattr is unavailable in the kernel.
This is achieved by the creation of an XattrClient interface and three implementations of this client
OSxattrClient - which wraps the kernel native xattr functions and is responsible for determining if xattr is available or not.
FilesystemBasedXattrClient - Which uses a directory parallel to the root to maintain metadata keys as individual files - one file per key
CompositeXattrClient - Which encapsulates the other two clients and dependent on the availability of xattr in the kernel will call the right client as applicable.
If we need to disable the fallback for any reason then the correct way to do this is to create a custom config map for assisted service.
```
apiVersion: v1
kind: ConfigMap
metadata:
name: custom-config-map
namespace: multicluster-engine
data:
ENABLE_XATTR_FALLBACK: "false"
```
This should then be added as an annotation to the AgentServiceConfig
```
kind: AgentServiceConfig
metadata:
annotations:
unsupported.agent-install.openshift.io/assisted-service-configmap: custom-config-map
```
Once you have updated the AgentServiceConfig, redeploy the service
```
oc rollout restart deployment -n multicluster-engine assisted-service
```
After the service redeploys, you should see an entry in the assisted service log to indicate that the setting has been picked up
```
oc logs -f assisted-service-66dfcbf689-95v5f -c asisted-service | grep -i FALLBACK
time="2024-12-03T09:46:36Z" level=info msg="Options.EnableXattrFallback:false" func=main.main file="/assisted-service/cmd/main.go:332"
```- Loading branch information
1 parent
973249d
commit ebb71ba
Showing
10 changed files
with
1,048 additions
and
282 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| package s3wrapper | ||
|
|
||
| import ( | ||
| "slices" | ||
|
|
||
| "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| func NewCompositeXattrClient( | ||
| log logrus.FieldLogger, | ||
| oSxattrClient XattrClient, | ||
| filesystemBasedXattrClient XattrClient, | ||
| ) (*CompositeXattrClient, error) { | ||
| oSxattrClientSupported, err := oSxattrClient.IsSupported() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if !oSxattrClientSupported { | ||
| log.Warn("The native xattr client doesn't support extended attributes. A fallback has been enabled.") | ||
| } | ||
| filesystemBasedXattrClientSupported, err := filesystemBasedXattrClient.IsSupported() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &CompositeXattrClient{ | ||
| oSxattrClient: oSxattrClient, | ||
| filesystemBasedXattrClient: filesystemBasedXattrClient, | ||
| oSxattrClientSupported: oSxattrClientSupported, | ||
| filesystemBasedXattrClientSupported: filesystemBasedXattrClientSupported, | ||
| }, nil | ||
| } | ||
|
|
||
| type CompositeXattrClient struct { | ||
| oSxattrClient XattrClient | ||
| filesystemBasedXattrClient XattrClient | ||
| oSxattrClientSupported bool | ||
| filesystemBasedXattrClientSupported bool | ||
| } | ||
|
|
||
| func (c *CompositeXattrClient) IsSupported() (bool, error) { | ||
| return true, nil | ||
| } | ||
|
|
||
| func (c *CompositeXattrClient) Set(tempFileName string, fileName string, key string, value string) error { | ||
| // If the native xattr writes are supported then use those | ||
| // otherwise fall back to filesystem based writes. | ||
| if c.oSxattrClientSupported { | ||
| return c.oSxattrClient.Set(tempFileName, fileName, key, value) | ||
| } | ||
| return c.filesystemBasedXattrClient.Set(tempFileName, fileName, key, value) | ||
| } | ||
|
|
||
| func (c *CompositeXattrClient) Get(path, key string) (string, bool, error) { | ||
| // Search for the record first in the oSxattrClient | ||
| // if not found then look in the filesystemBasedXattrClient. | ||
| var err error | ||
| var ok bool | ||
| var result string | ||
| if c.oSxattrClientSupported { | ||
| result, ok, err = c.oSxattrClient.Get(path, key) | ||
| if err != nil { | ||
| return "", ok, err | ||
| } | ||
| if ok { | ||
| return result, ok, nil | ||
| } | ||
| } | ||
| return c.filesystemBasedXattrClient.Get(path, key) | ||
| } | ||
|
|
||
| func (c *CompositeXattrClient) List(path string) ([]string, error) { | ||
| // Produce a list that is the union of keys from both clients | ||
| // Just in case the user has performed an upgrade from the filesystem based method. | ||
| // respect that the oSxattrClient takes priority when available. | ||
| var primaryList []string | ||
| var secondaryList []string | ||
| var err error | ||
| if c.oSxattrClientSupported { | ||
| primaryList, err = c.oSxattrClient.List(path) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| secondaryList, err = c.filesystemBasedXattrClient.List(path) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| for _, secondaryListItem := range secondaryList { | ||
| if !slices.Contains(primaryList, secondaryListItem) { | ||
| primaryList = append(primaryList, secondaryListItem) | ||
| } | ||
| } | ||
| return primaryList, nil | ||
| } | ||
|
|
||
| func (c *CompositeXattrClient) RemoveAll(path string) error { | ||
| // Metadata only needs to be removed in this way for the fallback solution. | ||
| return c.filesystemBasedXattrClient.RemoveAll(path) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| package s3wrapper | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
|
|
||
| "github.com/golang/mock/gomock" | ||
| "github.com/onsi/ginkgo" | ||
| . "github.com/onsi/ginkgo" | ||
| . "github.com/onsi/gomega" | ||
| "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| var _ = Describe("Composite Xattr Client", func() { | ||
|
|
||
| var ( | ||
| log *logrus.Logger | ||
| ctrl *gomock.Controller | ||
| baseDir string | ||
| osXattrClient *MockXattrClient | ||
| fileSystemBasedXattrClient XattrClient | ||
| file1path string | ||
| file2path string | ||
| ) | ||
|
|
||
| BeforeEach(func() { | ||
| var err error | ||
| log = logrus.New() | ||
| log.SetOutput(ginkgo.GinkgoWriter) | ||
| baseDir, err = os.MkdirTemp("", "test") | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| ctrl = gomock.NewController(GinkgoT()) | ||
| fileSystemBasedXattrClient = NewFilesystemBasedXattrClient(log, baseDir) | ||
| err = os.MkdirAll(filepath.Join(baseDir, "manifests", "openshift"), 0o700) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| err = os.MkdirAll(filepath.Join(baseDir, "manifests", "manifests"), 0o700) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| file1path = filepath.Join(baseDir, "manifests", "openshift", "file1.yaml") | ||
| file2path = filepath.Join(baseDir, "manifests", "manifests", "file2.yaml") | ||
| err = os.WriteFile(file1path, []byte{}, 0o600) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| err = os.WriteFile(file2path, []byte{}, 0o600) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| }) | ||
|
|
||
| getExpectedMetadataPath := func(filePath string, attributeName string) string { | ||
| relativePath := filePath[len(baseDir):] | ||
| return fmt.Sprintf("%s%s%s%s", filepath.Join(baseDir, filesystemXattrMetaDataDirectoryName, relativePath), delimiter, "user.", attributeName) | ||
| } | ||
|
|
||
| assertFileMetadataCorrect := func(filepath string, attributeName string, expectedValue string) { | ||
| fileMetadataItemPath := getExpectedMetadataPath(filepath, attributeName) | ||
| data, err := os.ReadFile(fileMetadataItemPath) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(string(data)).To(Equal(expectedValue)) | ||
| } | ||
|
|
||
| assertFileSystemBasedAttributeWrite := func(path string, attribute string, value string, compositeXattrClient XattrClient) { | ||
| err := compositeXattrClient.Set("", path, attribute, value) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| assertFileMetadataCorrect(path, attribute, value) | ||
| } | ||
|
|
||
| It("Upgrading to xattr supported natively after unsupported", func() { | ||
| var ( | ||
| compositeXattrClient XattrClient | ||
| ) | ||
|
|
||
| By("Native xattr is unsupported", func() { | ||
| osXattrClient = NewMockXattrClient(ctrl) | ||
| osXattrClient.EXPECT().IsSupported().Return(false, nil).AnyTimes() | ||
| }) | ||
| By("Create composite xattr client", func() { | ||
| var err error | ||
| compositeXattrClient, err = NewCompositeXattrClient(log, osXattrClient, fileSystemBasedXattrClient) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| }) | ||
| By("Filesystem xattr client should be used for writes", func() { | ||
| assertFileSystemBasedAttributeWrite(file1path, "arbitrary-attribute", "some-arbitrary-value", compositeXattrClient) | ||
| assertFileSystemBasedAttributeWrite(file1path, "another-arbitrary-attribute", "another-arbitrary-value", compositeXattrClient) | ||
| assertFileSystemBasedAttributeWrite(file2path, "arbitrary-attribute-a", "some-arbitrary-value-a", compositeXattrClient) | ||
| assertFileSystemBasedAttributeWrite(file2path, "arbitrary-attribute-b", "some-arbitrary-value-b", compositeXattrClient) | ||
| }) | ||
| By("Native xattr is now supported - simulated upgrade", func() { | ||
| var err error | ||
| compositeXattrClient, err = NewCompositeXattrClient(log, NewOSxAttrClient(log, baseDir), fileSystemBasedXattrClient) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| }) | ||
| By("Composite client should return previously stored keys from filesystem", func() { | ||
| value, ok, err := compositeXattrClient.Get(file1path, "arbitrary-attribute") | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(ok).To(BeTrue()) | ||
| Expect(value).To(Equal("some-arbitrary-value")) | ||
| }) | ||
| By("Composite client should write to os native xattr, leaving filesystem based xattr intact", func() { | ||
| err := compositeXattrClient.Set(file1path, file1path, "arbitrary-attribute", "a-new-value") | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| assertFileMetadataCorrect(file1path, "arbitrary-attribute", "some-arbitrary-value") | ||
| }) | ||
| By("should fetch newly written value from the composite client", func() { | ||
| value, ok, err := compositeXattrClient.Get(file1path, "arbitrary-attribute") | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(ok).To(BeTrue()) | ||
| Expect(value).To(Equal("a-new-value")) | ||
| }) | ||
| By("list from composite xattr client should merge keys", func() { | ||
| items, err := compositeXattrClient.List(file1path) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(items).To(ContainElement("arbitrary-attribute")) | ||
| Expect(items).To(ContainElement("another-arbitrary-attribute")) | ||
| }) | ||
| By("RemoveAll should remove filesystem based user keys", func() { | ||
| err := compositeXattrClient.Set(file2path, file2path, "additional-os-xattr", "some-value") | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| items, err := compositeXattrClient.List(file2path) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(items).To(ContainElement("arbitrary-attribute-a")) | ||
| Expect(items).To(ContainElement("arbitrary-attribute-a")) | ||
| Expect(items).To(ContainElement("additional-os-xattr")) | ||
| err = compositeXattrClient.RemoveAll(file2path) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| items, err = compositeXattrClient.List(file2path) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(items).ToNot(ContainElement("arbitrary-attribute-a")) | ||
| Expect(items).ToNot(ContainElement("arbitrary-attribute-a")) | ||
| // Placed here by the os native xattr for which we do not delete the keys in this way | ||
| // (xattr data is part of the file itself) | ||
| Expect(items).To(ContainElement("additional-os-xattr")) | ||
| }) | ||
| }) | ||
|
|
||
| AfterEach(func() { | ||
| ctrl.Finish() | ||
| err := os.RemoveAll(baseDir) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| }) | ||
|
|
||
| }) |
Oops, something went wrong.