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

sit-xfs: do not recreate fs #97

Merged
merged 1 commit into from
May 10, 2024
Merged

Conversation

spuiuk
Copy link
Collaborator

@spuiuk spuiuk commented May 7, 2024

This problem was hit when developing the playbook with backend left to the default xfs system. Attempting to re-run the playbook to test if some changes to the playbooks were working failed at the point of creating the xfs filesystem.

At the moment the playbook forces the creation of the xfs fs. This means that when re-running the ansible playbook, we attempt to recreate a filesystem which has already been mounted. This fails causing it to fail.

By setting to false, we do not force a recreation of the filesystem if one already exists. This makes the operation idempotent allowing us to re-run the playbook.

Another option is to unmount a mounted filesystem. However it will still wipe out any filesystems on it and is thus non-idempotent.

@spuiuk spuiuk requested review from xhernandez and anoopcs9 May 7, 2024 21:11
@@ -2,7 +2,7 @@
- name: Create the XFS filesystem
filesystem:
dev: "{{ volume.xfs.device }}"
force: true
force: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

false is the default in case you don't want to force the creation of filesystem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've seen defaults change with ansible. While this setting may not change, I see no harm in explicitly setting it to false.

@xhernandez
Copy link
Collaborator

Another option is to unmount a mounted filesystem. However it will still wipe out any filesystems on it and is thus non-idempotent.

I disagree here. If the filesystem is not wiped again, at the end of the playbook execution, the system state won't be the same that the initial execution of the playbook left.

Idempotent means that a given operation, even if executed many times, gives the same result as if it were executed only once. IMO the way to make it idempotent is to actually format it always.

Comment on lines 2 to 6
- name: Unmount the filesystem if already mounted
mount:
path: "{{ path }}"
state: unmounted

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be better to put this in the caller (playbooks/ansible/roles/samba.setup/tasks/main.yml). This way it will be useful for all backends (though some might need additional steps, but those could be added in future patches)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Xavi, I did some tests with the umount before mkfs.
I did the following

  1. Run "make" which brings up the cluster
  2. when the tests are running on the client, I interrupt and
  3. rerun make

At this point, I suspect samba still is actively operating on the test filesystem causing the unmount operation to fail. If we wait a while and re-run, it works successfully and completes.

To get around this, we need to stop samba before we attempt the unmount. Since this is handled by ctdb, it complicates matters.

I think the easier option for now to allow us to re-run make on the cluster is to simply use the option to not force the mkfs for xfs. I have pushed the original patches one more time. Let me know what you think.

At the moment the playbook forces the creation of the xfs fs. This means
that when re-running the ansible playbook, we attempt to recreate a
filesystem which has already been mounted. This fails causing it to
fail.

By setting to false, we do not force a recreation of the filesystem
which prevents this operation being idempotent.

Signed-off-by: Sachin Prabhu <[email protected]>
@spuiuk spuiuk merged commit 496213e into samba-in-kubernetes:main May 10, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants