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

Bug Fixes for Release 2.4.1 #1622

Merged
merged 22 commits into from
Feb 11, 2025
Merged

Bug Fixes for Release 2.4.1 #1622

merged 22 commits into from
Feb 11, 2025

Conversation

syeleti-msft
Copy link
Member

@syeleti-msft syeleti-msft commented Feb 4, 2025

  • Update renameFileOptions in fuse2 handler
  • Update the ETag for dst attribute while renaming the file
  • Sanitize the ETag in datalake before adding it to the attribute
  • Invalidate the attribute in attribute cache after doing a truncate operation on file. This is sort of quick hack done to avoid the complexity of truncate file in az storage component.
  • Add atomic_o_trunc flag in fuse2 options that will allow the fuse library to always pass the O_TRUNC flag on open call. see the comments in the code for more details
  • Send ETag as a struct field to the workitem, as directly retrieving etag from the handle inside the download method is not safe as it might cause a race. seems like Locking the handle to retrieve Etag inside the download method is also a bad idea, as we don't know the handle is already locked/unlocked.
  • When O_TRUNC flag is passed to the open call in file cache and closed immediately without any writes on the data, the old data inside the file is not getting wiped out from the server. fixed this issue
  • Remove usage of apt command in pipeline as it states that it doesn't have stable CLI interface to use in the scripts. Use always apt-get to install any packages.

@syeleti-msft syeleti-msft marked this pull request as ready for review February 5, 2025 14:00
@syeleti-msft syeleti-msft changed the title missing Changes for release Missing Changes for Release 2.4.2 Feb 6, 2025
@syeleti-msft syeleti-msft force-pushed the syeleti/missingReleaseChanges branch from ccea3cc to 09526c0 Compare February 6, 2025 13:15
@syeleti-msft syeleti-msft changed the title Missing Changes for Release 2.4.2 [WIP] Missing Changes for Release 2.4.2 Feb 7, 2025
@syeleti-msft syeleti-msft changed the title [WIP] Missing Changes for Release 2.4.2 Missing Changes for Release 2.4.2 Feb 8, 2025
@syeleti-msft syeleti-msft changed the title Missing Changes for Release 2.4.2 Bug Fixes for Release 2.4.2 Feb 8, 2025
Dst: dstPath,
SrcAttr: srcAttr,
DstAttr: dstAttr,
})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update renameFileOptions in fuse2 handler

// due to some changes that are required in az storage comp which
// were not necessarily required. Once they were done invalidation
// of the attribute can be removed.
ac.invalidatePath(options.Name)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invalidate the attribute in attribute cache after doing a truncate operation on file. This is sort of quick hack done to avoid the complexity of truncate file in az storage component.

@@ -400,7 +401,7 @@ func (dl *Datalake) GetAttr(name string) (blobAttr *internal.ObjAttr, err error)
Ctime: *prop.LastModified,
Crtime: *prop.LastModified,
Flags: internal.NewFileBitMap(),
ETag: (string)(*prop.ETag),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sanitize the ETag in datalake before adding it to the attribute

}

if copyStatus != nil && *copyStatus == blob.CopyStatusTypeSuccess {
modifyLMT(srcAttr, dstLMT)
modifyLMTandEtag(srcAttr, dstLMT, dstETag)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the ETag for dst attribute while renaming the file

// Not checking the options since we don't allow user to configure this flag.
// This is the default behaviour for the fuse3 hence we don't pass this flag there.
// ref: https://github.com/libfuse/libfuse/blob/7f86f3be7148b15b71b63357813c66dd32177cf6/lib/fuse_lowlevel.c#L2161C2-L2161C16
options += ",atomic_o_trunc"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add atomic_o_trunc flag in fuse2 options that will allow the fuse library to always pass the O_TRUNC flag on open call. see the comments in the code for more details

@@ -1055,8 +1061,7 @@ func (bc *BlockCache) download(item *workItem) {

// Compare the ETAG value and fail download if blob has changed
if etag != "" {
etagVal, found := item.handle.GetValue("ETAG")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Send ETag as a struct field to the workitem, as directly retrieving etag from the handle inside the download method is not safe as it might cause a race. seems like Locking the handle to retrieve Etag inside the download method is also a bad idea, as we don't know the handle is already locked/unlocked.

@@ -1042,6 +1042,9 @@ func (fc *FileCache) OpenFile(options internal.OpenFileOptions) (*handlemap.Hand
flock.Inc()

handle := handlemap.NewHandle(options.Name)
if options.Flags&os.O_TRUNC != 0 {
handle.Flags.Set(handlemap.HandleFlagDirty)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When O_TRUNC flag is passed to the open call in file cache and closed immediately without any writes on the data, the old data inside the file is not getting wiped out from the server. fixed this issue

@vibhansa-msft vibhansa-msft changed the title Bug Fixes for Release 2.4.2 Bug Fixes for Release 2.4.1 Feb 10, 2025
@vibhansa-msft vibhansa-msft added this to the v2-2.4.1 milestone Feb 10, 2025
@@ -938,12 +938,18 @@ func (bc *BlockCache) refreshBlock(handle *handlemap.Handle, index uint64, prefe

// lineupDownload : Create a work item and schedule the download
func (bc *BlockCache) lineupDownload(handle *handlemap.Handle, block *Block, prefetch bool) {
Copy link
Collaborator

@jainakanksha-msft jainakanksha-msft Feb 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kidly add junit test case, where in case of lineing up we need to set Etag.

@syeleti-msft syeleti-msft merged commit be07ce3 into main Feb 11, 2025
10 checks passed
@syeleti-msft syeleti-msft deleted the syeleti/missingReleaseChanges branch February 11, 2025 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants