feat: obviate ghostlinkd (ietf-tools#7336)

* wip: identify whats needed to obviate ghostlinkd

* fix: hardlink new charter files to ftp directory

* fix: hardlink new charter files to ftp directory (continued)

* chore: bring settings comment up to date

* chore: add archive and ftp dirs to setup of various environments

* fix: test charter submits write to ftp dir

* chore: remove debug

* fix: test charter approval writes to ftp dir

* fix: link review revisions into ftp dir

* fix: link to all archive and ftp on submission post

* chore: clean comments, move action to github issue

* fix: link idindex files to all archive and ftp

* chore: deflake

* chore: remove TODO comment

* fix: use settings

* chore: rename new setting

Author: rjsparks

dev/deploy-to-container/settings_local.py

@@ -60,10 +60,11 @@
 BOFREQ_PATH = '/assets/ietf-ftp/bofreq/'
 CONFLICT_REVIEW_PATH = '/assets/ietf-ftp/conflict-reviews/'
 STATUS_CHANGE_PATH = '/assets/ietf-ftp/status-changes/'
-INTERNET_DRAFT_ARCHIVE_DIR = '/assets/archive/id'
+INTERNET_DRAFT_ARCHIVE_DIR = '/assets/collection/draft-archive'
 INTERNET_ALL_DRAFTS_ARCHIVE_DIR = '/assets/archive/id'
 BIBXML_BASE_PATH = '/assets/ietfdata/derived/bibxml'
 IDSUBMIT_REPOSITORY_PATH = INTERNET_DRAFT_PATH
+FTP_DIR = '/assets/ftp'
 
 NOMCOM_PUBLIC_KEYS_DIR = 'data/nomcom_keys/public_keys/'
 SLIDE_STAGING_PATH = '/test/staging/'

dev/diff/settings_local.py

@@ -57,9 +57,10 @@
 BOFREQ_PATH = '/assets/ietf-ftp/bofreq/'
 CONFLICT_REVIEW_PATH = '/assets/ietf-ftp/conflict-reviews/'
 STATUS_CHANGE_PATH = '/assets/ietf-ftp/status-changes/'
-INTERNET_DRAFT_ARCHIVE_DIR = '/assets/ietf-ftp/internet-drafts/'
+INTERNET_DRAFT_ARCHIVE_DIR = '/assets/collection/draft-archive'
 INTERNET_ALL_DRAFTS_ARCHIVE_DIR = '/assets/ietf-ftp/internet-drafts/'
 BIBXML_BASE_PATH = '/assets/ietfdata/derived/bibxml'
+FTP_DIR = '/assets/ftp'
 
 NOMCOM_PUBLIC_KEYS_DIR = 'data/nomcom_keys/public_keys/'
 SLIDE_STAGING_PATH = 'test/staging/'

dev/tests/settings_local.py

@@ -56,9 +56,10 @@
 BOFREQ_PATH = '/assets/ietf-ftp/bofreq/'
 CONFLICT_REVIEW_PATH = '/assets/ietf-ftp/conflict-reviews/'
 STATUS_CHANGE_PATH = '/assets/ietf-ftp/status-changes/'
-INTERNET_DRAFT_ARCHIVE_DIR = '/assets/ietf-ftp/internet-drafts/'
+INTERNET_DRAFT_ARCHIVE_DIR = '/assets/collection/draft-archive'
 INTERNET_ALL_DRAFTS_ARCHIVE_DIR = '/assets/ietf-ftp/internet-drafts/'
 BIBXML_BASE_PATH = '/assets/ietfdata/derived/bibxml'
+FTP_DIR = '/assets/ftp'
 
 NOMCOM_PUBLIC_KEYS_DIR = 'data/nomcom_keys/public_keys/'
 SLIDE_STAGING_PATH = 'test/staging/'

docker/configs/settings_local.py

@@ -46,10 +46,11 @@
 BOFREQ_PATH = '/assets/ietf-ftp/bofreq/'
 CONFLICT_REVIEW_PATH = '/assets/ietf-ftp/conflict-reviews/'
 STATUS_CHANGE_PATH = '/assets/ietf-ftp/status-changes/'
-INTERNET_DRAFT_ARCHIVE_DIR = '/assets/archive/id'
+INTERNET_DRAFT_ARCHIVE_DIR = '/assets/collection/draft-archive'
 INTERNET_ALL_DRAFTS_ARCHIVE_DIR = '/assets/archive/id'
 BIBXML_BASE_PATH = '/assets/ietfdata/derived/bibxml'
 IDSUBMIT_REPOSITORY_PATH = INTERNET_DRAFT_PATH
+FTP_DIR = '/assets/ftp'
 
 NOMCOM_PUBLIC_KEYS_DIR = 'data/nomcom_keys/public_keys/'
 SLIDE_STAGING_PATH = 'test/staging/'

docker/scripts/app-create-dirs.sh

@@ -9,6 +9,8 @@ for sub in \
     test/wiki/ietf \
     data/nomcom_keys/public_keys \
     /assets/archive/id \
+    /assets/collection \
+    /assets/collection/draft-archive \
     /assets/ietf-ftp \
     /assets/ietf-ftp/bofreq \
     /assets/ietf-ftp/charter \
@@ -33,6 +35,10 @@ for sub in \
     /assets/www6/iesg \
     /assets/www6/iesg/evaluation \
     /assets/media/photo \
+    /assets/ftp \
+    /assets/ftp/charter \
+    /assets/ftp/internet-drafts \
+    /assets/ftp/review \
     ; do
     if [ ! -d "$sub"  ]; then
         echo "Creating dir $sub"

ietf/doc/expire.py

@@ -139,6 +139,9 @@ def move_file(f):
 
         if os.path.exists(src):
             try:
+                # ghostlinkd would keep this in the combined all archive since it would
+                # be sourced from a different place. But when ghostlinkd is removed, nothing
+                # new is needed here - the file will already exist in the combined archive
                 shutil.move(src, dst)
             except IOError as e:
                 if "No such file or directory" in str(e):
@@ -213,6 +216,10 @@ def splitext(fn):
             filename, revision = match.groups()
 
         def move_file_to(subdir):
+            # Similar to move_draft_files_to_archive
+            # ghostlinkd would keep this in the combined all archive since it would
+            # be sourced from a different place. But when ghostlinkd is removed, nothing
+            # new is needed here - the file will already exist in the combined archive
             shutil.move(path,
                         os.path.join(settings.INTERNET_DRAFT_ARCHIVE_DIR, subdir, basename))
 
@@ -229,4 +236,5 @@ def move_file_to(subdir):
                 move_file_to("")
 
         except Document.DoesNotExist:
+            # All uses of this past 2014 seem related to major system failures.
             move_file_to("unknown_ids")

ietf/doc/models.py

@@ -142,6 +142,7 @@ def get_file_path(self):
                 if self.is_dochistory():
                     self._cached_file_path = settings.INTERNET_ALL_DRAFTS_ARCHIVE_DIR
                 else:
+                    # This could be simplified since anything in INTERNET_DRAFT_PATH is also already in INTERNET_ALL_DRAFTS_ARCHIVE_DIR
                     draft_state = self.get_state('draft')
                     if draft_state and draft_state.slug == 'active':
                         self._cached_file_path = settings.INTERNET_DRAFT_PATH

ietf/doc/tests_charter.py

@@ -87,6 +87,10 @@ def test_view_revisions(self):
 class EditCharterTests(TestCase):
     settings_temp_path_overrides = TestCase.settings_temp_path_overrides + ['CHARTER_PATH']
 
+    def setUp(self):
+        super().setUp()
+        (Path(settings.FTP_DIR)/"charter").mkdir()
+
     def write_charter_file(self, charter):
         (Path(settings.CHARTER_PATH) / f"{charter.name}-{charter.rev}.txt").write_text("This is a charter.")
 
@@ -506,13 +510,16 @@ def test_submit_charter(self):
         self.assertEqual(charter.rev, next_revision(prev_rev))
         self.assertTrue("new_revision" in charter.latest_event().type)
 
-        file_contents = (
-            Path(settings.CHARTER_PATH) / (charter.name + "-" + charter.rev + ".txt")
-        ).read_text("utf-8")
+        charter_path = Path(settings.CHARTER_PATH) / (charter.name + "-" + charter.rev + ".txt")
+        file_contents = (charter_path).read_text("utf-8")
         self.assertEqual(
             file_contents,
             "Windows line\nMac line\nUnix line\n" + utf_8_snippet.decode("utf-8"),
         )
+        ftp_charter_path = Path(settings.FTP_DIR) / "charter" / charter_path.name
+        self.assertTrue(ftp_charter_path.exists())
+        self.assertTrue(charter_path.samefile(ftp_charter_path))
+
 
     def test_submit_initial_charter(self):
         group = GroupFactory(type_id='wg',acronym='mars',list_email='mars-wg@ietf.org')
@@ -808,9 +815,11 @@ def test_approve(self):
         self.assertTrue(not charter.ballot_open("approve"))
 
         self.assertEqual(charter.rev, "01")
-        self.assertTrue(
-            (Path(settings.CHARTER_PATH) / ("charter-ietf-%s-%s.txt" % (group.acronym, charter.rev))).exists()
-        )
+        charter_path = Path(settings.CHARTER_PATH) / ("charter-ietf-%s-%s.txt" % (group.acronym, charter.rev))
+        charter_ftp_path = Path(settings.FTP_DIR) / "charter" / charter_path.name
+        self.assertTrue(charter_path.exists())
+        self.assertTrue(charter_ftp_path.exists())
+        self.assertTrue(charter_path.samefile(charter_ftp_path))
 
         self.assertEqual(len(outbox), 2)
         #

ietf/doc/tests_review.py

@@ -2,6 +2,7 @@
 # -*- coding: utf-8 -*-
 
 
+from pathlib import Path
 import datetime, os, shutil
 import io
 import tarfile, tempfile, mailbox
@@ -47,6 +48,7 @@ def setUp(self):
         self.review_dir = self.tempdir('review')
         self.old_document_path_pattern = settings.DOCUMENT_PATH_PATTERN
         settings.DOCUMENT_PATH_PATTERN = self.review_dir + "/{doc.type_id}/"
+        (Path(settings.FTP_DIR) / "review").mkdir()
 
         self.review_subdir = os.path.join(self.review_dir, "review")
         if not os.path.exists(self.review_subdir):
@@ -57,6 +59,13 @@ def tearDown(self):
         settings.DOCUMENT_PATH_PATTERN = self.old_document_path_pattern
         super().tearDown()
 
+    def verify_review_files_were_written(self, assignment, expected_content = "This is a review\nwith two lines"):
+        review_file = Path(self.review_subdir) / f"{assignment.review.name}.txt"
+        content = review_file.read_text()
+        self.assertEqual(content, expected_content)
+        review_ftp_file = Path(settings.FTP_DIR) / "review" / review_file.name
+        self.assertTrue(review_file.samefile(review_ftp_file))
+
     def test_request_review(self):
         doc = WgDraftFactory(group__acronym='mars',rev='01')
         NewRevisionDocEventFactory(doc=doc,rev='01')
@@ -830,8 +839,7 @@ def test_complete_review_upload_content(self):
         self.assertTrue(assignment.review_request.team.acronym.lower() in assignment.review.name)
         self.assertTrue(assignment.review_request.doc.rev in assignment.review.name)
 
-        with io.open(os.path.join(self.review_subdir, assignment.review.name + ".txt")) as f:
-            self.assertEqual(f.read(), "This is a review\nwith two lines")
+        self.verify_review_files_were_written(assignment)
 
         self.assertEqual(len(outbox), 1)
         self.assertIn(assignment.review_request.team.list_email, outbox[0]["To"])
@@ -885,8 +893,7 @@ def test_complete_review_enter_content(self):
         completed_time_diff = timezone.now() - assignment.completed_on
         self.assertLess(completed_time_diff, datetime.timedelta(seconds=10))
 
-        with io.open(os.path.join(self.review_subdir, assignment.review.name + ".txt")) as f:
-            self.assertEqual(f.read(), "This is a review\nwith two lines")
+        self.verify_review_files_were_written(assignment)
 
         self.assertEqual(len(outbox), 1)
         self.assertIn(assignment.review_request.team.list_email, outbox[0]["To"])
@@ -926,8 +933,7 @@ def test_complete_review_enter_content_by_secretary(self):
         self.assertLess(event0_time_diff, datetime.timedelta(seconds=10))
         self.assertEqual(events[1].time, datetime.datetime(2012, 12, 24, 12, 13, 14, tzinfo=DEADLINE_TZINFO))
 
-        with io.open(os.path.join(self.review_subdir, assignment.review.name + ".txt")) as f:
-            self.assertEqual(f.read(), "This is a review\nwith two lines")
+        self.verify_review_files_were_written(assignment)
 
         self.assertEqual(len(outbox), 1)
         self.assertIn(assignment.review_request.team.list_email, outbox[0]["To"])
@@ -1013,8 +1019,7 @@ def test_complete_review_link_to_mailing_list(self, mock):
         assignment = reload_db_objects(assignment)
         self.assertEqual(assignment.state_id, "completed")
 
-        with io.open(os.path.join(self.review_subdir, assignment.review.name + ".txt")) as f:
-            self.assertEqual(f.read(), "This is a review\nwith two lines")
+        self.verify_review_files_were_written(assignment)
 
         self.assertEqual(len(outbox), 0)
         self.assertTrue("http://example.com" in assignment.review.external_url)
@@ -1063,8 +1068,7 @@ def test_complete_unsolicited_review_link_to_mailing_list_by_secretary(self, moc
         self.assertEqual(assignment.reviewer, rev_role.person.role_email('reviewer'))
         self.assertEqual(assignment.state_id, "completed")
 
-        with io.open(os.path.join(self.review_subdir, assignment.review.name + ".txt")) as f:
-            self.assertEqual(f.read(), "This is a review\nwith two lines")
+        self.verify_review_files_were_written(assignment)
 
         self.assertEqual(len(outbox), 0)
         self.assertTrue("http://example.com" in assignment.review.external_url)
@@ -1172,8 +1176,9 @@ def test_revise_review_enter_content(self):
         self.assertLess(event_time_diff, datetime.timedelta(seconds=10))
         self.assertTrue('revised' in event1.desc.lower())
 
-        with io.open(os.path.join(self.review_subdir, assignment.review.name + ".txt")) as f:
-            self.assertEqual(f.read(), "This is a review\nwith two lines")
+        # See https://github.com/ietf-tools/datatracker/issues/6941
+        # These are _not_ getting written as a new version as intended.
+        self.verify_review_files_were_written(assignment)
 
         self.assertEqual(len(outbox), 0)
 
@@ -1200,6 +1205,8 @@ def test_revise_review_enter_content(self):
         # Ensure that a new event was created for the new revision (#2590)
         self.assertNotEqual(event1.id, event2.id)
 
+        self.verify_review_files_were_written(assignment, "This is a revised review")
+
         self.assertEqual(len(outbox), 0)
 
     def test_edit_comment(self):

ietf/doc/utils_charter.py

@@ -92,11 +92,31 @@ def change_group_state_after_charter_approval(group, by):
 def fix_charter_revision_after_approval(charter, by):
     # according to spec, 00-02 becomes 01, so copy file and record new revision
     try:
-        old = os.path.join(charter.get_file_path(), '%s-%s.txt' % (charter.name, charter.rev))
-        new = os.path.join(charter.get_file_path(), '%s-%s.txt' % (charter.name, next_approved_revision(charter.rev)))
+        old = os.path.join(
+            charter.get_file_path(), "%s-%s.txt" % (charter.name, charter.rev)
+        )
+        new = os.path.join(
+            charter.get_file_path(),
+            "%s-%s.txt" % (charter.name, next_approved_revision(charter.rev)),
+        )
         shutil.copy(old, new)
     except IOError:
         log("There was an error copying %s to %s" % (old, new))
+    # Also provide a copy to the legacy ftp source directory, which is served by rsync
+    # This replaces the hardlink copy that ghostlink has made in the past
+    # Still using a hardlink as long as these are on the same filesystem.
+    # Staying with os.path vs pathlib.Path until we get to python>=3.10.
+    charter_dir = os.path.join(settings.FTP_DIR, "charter")
+    ftp_filepath = os.path.join(
+        charter_dir, "%s-%s.txt" % (charter.name, next_approved_revision(charter.rev))
+    )
+    try:
+        os.link(new, ftp_filepath)
+    except IOError:
+        log(
+            "There was an error creating a harlink at %s pointing to %s"
+            % (ftp_filepath, new)
+        )
 
     events = []
     e = NewRevisionDocEvent(doc=charter, by=by, type="new_revision")
@@ -108,6 +128,7 @@ def fix_charter_revision_after_approval(charter, by):
     charter.rev = e.rev
     charter.save_with_history(events)
 
+
 def historic_milestones_for_charter(charter, rev):
     """Return GroupMilestone/GroupMilestoneHistory objects for charter
     document at rev by looking through the history."""