-
Notifications
You must be signed in to change notification settings - Fork 217
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
OCPBUGS-44849: Skip manifest metadata operations if xattr support not…
… present. 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. As an intermim measure, it has been decided that we will skip manifest metadata operations and warn the user if we find that xattr is unsupported for the file path.
- Loading branch information
1 parent
8dc67a1
commit 4f9c79c
Showing
9 changed files
with
1,036 additions
and
293 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,122 @@ | ||
| package s3wrapper | ||
|
|
||
| import ( | ||
| "slices" | ||
|
|
||
| "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| func NewCompositeXattrClient( | ||
| log logrus.FieldLogger, | ||
| oSxattrClient XattrClient, | ||
| filesystemBasedXattrClient XattrClient, | ||
| rootDirectory string, | ||
| ) (*CompositeXattrClient, error) { | ||
| oSxattrClientSupported, err := oSxattrClient.IsSupported() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| filesystemBasedXattrClientSupported, err := filesystemBasedXattrClient.IsSupported() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &CompositeXattrClient{ | ||
| rootDirectory: rootDirectory, | ||
| oSxattrClient: oSxattrClient, | ||
| filesystemBasedXattrClient: filesystemBasedXattrClient, | ||
| oSxattrClientSupported: oSxattrClientSupported, | ||
| filesystemBasedXattrClientSupported: filesystemBasedXattrClientSupported, | ||
| }, nil | ||
| } | ||
|
|
||
| type CompositeXattrClient struct { | ||
| rootDirectory string | ||
| oSxattrClient XattrClient | ||
| filesystemBasedXattrClient XattrClient | ||
| oSxattrClientSupported bool | ||
| filesystemBasedXattrClientSupported bool | ||
| } | ||
|
|
||
| func (c *CompositeXattrClient) IsSupported() (bool, error) { | ||
| return true, nil | ||
| } | ||
|
|
||
| func (c *CompositeXattrClient) SetRootDirectory(path string) { | ||
| c.rootDirectory = path | ||
| } | ||
| func (c *CompositeXattrClient) GetRootDirectory() string { | ||
| return c.rootDirectory | ||
| } | ||
|
|
||
| func (c *CompositeXattrClient) Set(path, 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(path, key, value) | ||
| } | ||
| return c.filesystemBasedXattrClient.Set(path, 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 | ||
| // respect that the oSxattrClient takes priority when available. | ||
| var primaryList []string | ||
| var tertiarayList []string | ||
| var err error | ||
| if c.oSxattrClientSupported { | ||
| primaryList, err = c.oSxattrClient.List(path) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| tertiarayList, err = c.filesystemBasedXattrClient.List(path) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| for _, tertiarayListItem := range tertiarayList { | ||
| if !slices.Contains(primaryList, tertiarayListItem) { | ||
| primaryList = append(primaryList, tertiarayListItem) | ||
| } | ||
| } | ||
| return primaryList, nil | ||
| } | ||
|
|
||
| func (c *CompositeXattrClient) Remove(path string, key string) error { | ||
| // When asked to remove an item, we will remove using both clients if possible. | ||
| if c.oSxattrClientSupported { | ||
| err := c.oSxattrClient.Remove(path, key) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return c.filesystemBasedXattrClient.Remove(path, key) | ||
| } | ||
|
|
||
| func (c *CompositeXattrClient) RemoveAll(path string) error { | ||
| // When asked to remove all items for a pth, we will remove using both clients if possible. | ||
| if c.oSxattrClientSupported { | ||
| err := c.oSxattrClient.RemoveAll(path) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| 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,150 @@ | ||
| 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, baseDir) | ||
| 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, baseDir) | ||
| 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, "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("when key that only exists in filesystem is removed - it should be removed", func() { | ||
| err := compositeXattrClient.Remove(file1path, "another-arbitrary-attribute") | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| items, err := compositeXattrClient.List(file1path) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(items).To(ContainElement("arbitrary-attribute")) | ||
| Expect(items).ToNot(ContainElement("another-arbitrary-attribute")) | ||
| }) | ||
| By("when key is removed, should be removed from os xattr and filesystem xattr", func() { | ||
| err := compositeXattrClient.Remove(file1path, "arbitrary-attribute") | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| items, err := compositeXattrClient.List(file1path) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(items).ToNot(ContainElement("arbitrary-attribute")) | ||
| }) | ||
| By("RemoveAll should remove all keys", func() { | ||
| err := compositeXattrClient.Set(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(len(items)).To(Equal(0)) | ||
| }) | ||
| }) | ||
|
|
||
| AfterEach(func() { | ||
| ctrl.Finish() | ||
| err := os.RemoveAll(baseDir) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| }) | ||
|
|
||
| }) |
Oops, something went wrong.